Re: [RFC PULL] Add Display Support for Qualcomm SDM845

[Rob Clark]

On Wed, Feb 14, 2018 at 2:08 PM, Rob Clark  wrote:
> On Wed, Feb 14, 2018 at 1:17 PM, jsanka  wrote:
>> On 2/13/2018 12:00 PM, Rob Clark wrote:
>>>
>>>   - It looks like, as was the case with mdp4/mdp5 (and really most other
>>> hw)
>>> there are still unequal capabilities for planes (ie. some can do YUV,
>>> scaling, etc).
>>>
>>> But drm-hwc (or weston) isn't going to know about that, so I think
>>> we'll
>>> need to add support for dynamically assigning dpu_plane::pipe, similar
>>> to what mdp5 does w/ plane<->hwpipe.  (Which I actually added
>>> specifically
>>> for drm-hwc ;-))
>>
>> We are working on plane virtualization focused primarily to support 4K /
>> wider displays which cannot
>> be catered by one hwpipe. The plan is to gang up two *fixed* hwpipes of
>> similar capabilities (in terms of formats,
>> sub hw blocks like scalar, post processing ) and expose them as a single
>> plane so that user space
>> compositor ( drm-hwc or weston) need not worry about max pixel width
>> supported by the planes. But mapping
>> planes <-> hwpipe dynamically may need more than just virtualization. Kernel
>> need to keep track of hwpipes
>> especially when dealing with multiple displays. And it get complicated when
>> planes start moving around CRTC's
>> for different sized buffers.
>
> Hmm, a fixed mapping of hw pipe to plane might be an ok stepping
> stone.  I'm not sure it is a terribly good final solution, esp. when
> it comes to external displays, since you may never need 4k+ scanout,
> depending on what the user plugs in, so you end up wasting a lot of hw
> pipes.
>
> Keeping track of the hwpipes as part of the driver global atomic state
> isn't actually *that* hard.  Have a look at what mdp5 does.  We still
> need to move mdp5 to drm_private_obj instead of subclassing
> drm_atomic_state (see Archit's RFC, "drm/msm/mdp5: Add global state as
> a private atomic object"), but other than that detail, I think
> something along those lines is a better approach.
>

One additional point that I thought about, while implementing
writeback on mdp5.. I think with a cmd-mode panel (and dp psr?) we
could re-use idle hwpipes (ie. while not pushing an update out to the
panel) with a different crtc (LM) and writeback connector to flatten
all the layers to a single buffer while the screen is static, without
having to remove the drm planes from the crtc.  I think that would be
a lot less confusing than figuring out somehow that removing a plane
from a crtc shouldn't be flushed out to the panel.

BR,
-R
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC PULL] Add Display Support for Qualcomm SDM845

[Daniel Vetter]

On Tue, Feb 13, 2018 at 03:00:09PM -0500, Rob Clark wrote:
> On Tue, Feb 13, 2018 at 2:18 PM, Sean Paul  wrote:
> > Hi dri-devel,
> > Qualcomm has been working for the past few weeks on forward porting their
> > downstream drm driver from 4.14 to mainline. Please consider this PR as a
> > request for review, rather than an attempt at mainlining the code as it
> > currently stands. The goal is get this driver in shape over the next coming
> > months.
> >
> > In the meantime, I'll be hosting a tree here [1] to stage the fixes. Patches
> > will be posted and reviewed on linux-arm-...@vger.kernel.org. Once things 
> > look
> > good, I'll send another pull 4realz.
> >
> > To get the ball rolling, I've done some review on the new connector code, my
> > comments are below.
> >
> > Thanks in advance for your constructive feedback :)
> >
> > Sean
> >
> > [1]- git://people.freedesktop.org/~seanpaul/dpu-staging
> >
> > Review feedback:
> > 
> 
> just so others aren't confused, s/sde/dpu/g in the list below (we did
> our DAL->DC rename before sending first RFC :-P)
> 
> > - Solve the splash screen handling (or remove it)
> > - Simplify devicetree binding (remove register offsets)
> > feedback from reviewing sde_connector.c:
> > - Rationalize backlight implementation in sde_connector (display_count 
> > static)
> > - Sort out the dsi event passing between dsi/encoder/connector (move to 
> > encoder)
> > - include/uapi/drm/msm_drm_pp.h needs opensource userspace (or removal)
> > - connector->state access violations reading/writing mode_info
> > - s/sde_rect/drm_rect/
> > - sde_kms_info usage needs to be replaced with formal data structures (not
> >   stringified keypairs)
> > - sde_connector_ops needs to be trimmed, duplicates connector helpers, info
> >   hooks circumvent state, and other hooks should be stored in state or
> >   prepopulated (get_dst_format)
> > - sde_connector_get_dpms unused
> > - esd status check should migrate to encoder from connector
> > - backlight should be handled in panel drivers, not in the generic 
> > connector/dsi
> >   encoder
> > - sde_connector_helper_bridge_disable is called from encoder and calls back 
> > into
> >   set_power encoder function. if backlight, and esd status are removed,
> >   pre_kickoff can probably go away
> > - sde_connector_clk_ctrl is another example of encoder->connector->encoder 
> > call
> > - RETIRE_FENCE connector property should be removed, opting for the native
> >   atomic fences
> > - ROI (regions of interest) should be expressed per-plane instead of 
> > connector.
> >   there is work ongoing to support dirty_rects per-plane by Deepak Singh 
> > Rawat
> >    and Lukasz Spintzyk 
> > - Uma Shankar  has proposed HDR source metadata
> >   properties on the list, we should pivot to those instead of hand-rolling 
> > them
> >   in the sde driver
> > - Convert HDCP implementation to upstream Content Protection property
> > - Merge dsi and dsi_staging into one driver
> > - Writeback connector has been proposed by ARM (Liviu Dudau and Brian 
> > Starkey),
> >   we should work with their proposal instead of rolling OUT_FB ourselves
> > - sde_connector_set_property should be replaced with atomic helper
> > - dsi hotplug can probably be punted to the panel driver
> > - dpms should switch to enable/disable (or at least use the atomic helpers)
> > - dsi mode handling should also defer to the panel driver
> > - SDE_WB_CONFIG ioctl should be removed in favor of the existing ioctl to 
> > add
> >   user-defined modes
> > - dp implementation should use the existing dp helpers wherever possible
> > - lots of duplicated structures in dsi_defs.h that can be replaced with 
> > existing
> >   drm structs
> > - mode_valid should be split up and implemented directly in 
> > connector/encoder as
> >   appropriate
> > - sde_connector->aspace seems like it's unused?
> >
> 
> To add to that, a few things I've noticed (but I've mostly only gotten
> as far as the front-end of the pipeline, ie. dpu_plane):
> 
>  - It looks like, as was the case with mdp4/mdp5 (and really most other hw)
>there are still unequal capabilities for planes (ie. some can do YUV,
>scaling, etc).
> 
>But drm-hwc (or weston) isn't going to know about that, so I think we'll
>need to add support for dynamically assigning dpu_plane::pipe, similar
>to what mdp5 does w/ plane<->hwpipe.  (Which I actually added specifically
>for drm-hwc ;-))
> 
>  - I *think* we also need the same trick for combining mixers under one CRTC
>for 4k modes too?

Sounds like this should reuse the mdp5 framework in msm, at least in
parts? Reinventing the same virtualization will get boring :-)

>  - in dpu_plane, and perhaps elsewhere, userspace pointers for things like CSC
>and scaling coefficients need to be annotated w/ __user.  (Except the 
> should
>probably be blob properties 

Re: [RFC PULL] Add Display Support for Qualcomm SDM845

[Rob Herring]

On Tue, Feb 13, 2018 at 1:18 PM, Sean Paul  wrote:
> Hi dri-devel,
> Qualcomm has been working for the past few weeks on forward porting their
> downstream drm driver from 4.14 to mainline. Please consider this PR as a
> request for review, rather than an attempt at mainlining the code as it
> currently stands. The goal is get this driver in shape over the next coming
> months.
>
> In the meantime, I'll be hosting a tree here [1] to stage the fixes. Patches
> will be posted and reviewed on linux-arm-...@vger.kernel.org. Once things look
> good, I'll send another pull 4realz.
>
> To get the ball rolling, I've done some review on the new connector code, my
> comments are below.
>
> Thanks in advance for your constructive feedback :)


>  .../devicetree/bindings/display/msm/dpu-rsc.txt|   96 +
>  .../devicetree/bindings/display/msm/dpu.txt|  736 +++
>  .../devicetree/bindings/display/msm/dsi.txt|   16 +

>  .../devicetree/bindings/drm/msm/dpu-dp.txt |  217 +
>  .../devicetree/bindings/drm/msm/dpu-dsi.txt|  102 +
>  .../devicetree/bindings/drm/msm/dpu-wb.txt |   23 +
>  .../devicetree/bindings/drm/msm/mdss-dsi-panel.txt |  772 +++

Not a correct path with /drm/.

772 lines for a panel or DSI!? I look at it when it is shorter...

>  .../devicetree/bindings/fb/mdss-dsi-panel.txt  |  742 +++

Hopefully just duplicated?

>  Documentation/devicetree/bindings/fb/mdss-pll.txt  |  103 +

Not a correct path either.

>  .../devicetree/bindings/msm_hdcp/msm_hdcp.txt  |   14 +

ditto.

Rob
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC PULL] Add Display Support for Qualcomm SDM845

[jsanka]

On 2/13/2018 12:00 PM, Rob Clark wrote:

On Tue, Feb 13, 2018 at 2:18 PM, Sean Paul  wrote:

Hi dri-devel,
Qualcomm has been working for the past few weeks on forward porting their
downstream drm driver from 4.14 to mainline. Please consider this PR as a
request for review, rather than an attempt at mainlining the code as it
currently stands. The goal is get this driver in shape over the next coming
months.

In the meantime, I'll be hosting a tree here [1] to stage the fixes. Patches
will be posted and reviewed on linux-arm-...@vger.kernel.org. Once things look
good, I'll send another pull 4realz.

To get the ball rolling, I've done some review on the new connector code, my
comments are below.

Thanks in advance for your constructive feedback :)

Sean

[1]- git://people.freedesktop.org/~seanpaul/dpu-staging

Review feedback:


just so others aren't confused, s/sde/dpu/g in the list below (we did
our DAL->DC rename before sending first RFC :-P)


- Solve the splash screen handling (or remove it)
- Simplify devicetree binding (remove register offsets)
feedback from reviewing sde_connector.c:
- Rationalize backlight implementation in sde_connector (display_count static)
- Sort out the dsi event passing between dsi/encoder/connector (move to encoder)
- include/uapi/drm/msm_drm_pp.h needs opensource userspace (or removal)
- connector->state access violations reading/writing mode_info
- s/sde_rect/drm_rect/
- sde_kms_info usage needs to be replaced with formal data structures (not
   stringified keypairs)
- sde_connector_ops needs to be trimmed, duplicates connector helpers, info
   hooks circumvent state, and other hooks should be stored in state or
   prepopulated (get_dst_format)
- sde_connector_get_dpms unused
- esd status check should migrate to encoder from connector
- backlight should be handled in panel drivers, not in the generic connector/dsi
   encoder
- sde_connector_helper_bridge_disable is called from encoder and calls back into
   set_power encoder function. if backlight, and esd status are removed,
   pre_kickoff can probably go away
- sde_connector_clk_ctrl is another example of encoder->connector->encoder call
- RETIRE_FENCE connector property should be removed, opting for the native
   atomic fences
- ROI (regions of interest) should be expressed per-plane instead of connector.
   there is work ongoing to support dirty_rects per-plane by Deepak Singh Rawat
    and Lukasz Spintzyk 
- Uma Shankar  has proposed HDR source metadata
   properties on the list, we should pivot to those instead of hand-rolling them
   in the sde driver
- Convert HDCP implementation to upstream Content Protection property
- Merge dsi and dsi_staging into one driver
- Writeback connector has been proposed by ARM (Liviu Dudau and Brian Starkey),
   we should work with their proposal instead of rolling OUT_FB ourselves
- sde_connector_set_property should be replaced with atomic helper
- dsi hotplug can probably be punted to the panel driver
- dpms should switch to enable/disable (or at least use the atomic helpers)
- dsi mode handling should also defer to the panel driver
- SDE_WB_CONFIG ioctl should be removed in favor of the existing ioctl to add
   user-defined modes
- dp implementation should use the existing dp helpers wherever possible
- lots of duplicated structures in dsi_defs.h that can be replaced with existing
   drm structs
- mode_valid should be split up and implemented directly in connector/encoder as
   appropriate
- sde_connector->aspace seems like it's unused?


To add to that, a few things I've noticed (but I've mostly only gotten
as far as the front-end of the pipeline, ie. dpu_plane):

  - It looks like, as was the case with mdp4/mdp5 (and really most other hw)
there are still unequal capabilities for planes (ie. some can do YUV,
scaling, etc).

But drm-hwc (or weston) isn't going to know about that, so I think we'll
need to add support for dynamically assigning dpu_plane::pipe, similar
to what mdp5 does w/ plane<->hwpipe.  (Which I actually added specifically
for drm-hwc ;-))
We are working on plane virtualization focused primarily to support 4K / 
wider displays which cannot
be catered by one hwpipe. The plan is to gang up two *fixed* hwpipes of 
similar capabilities (in terms of formats,
sub hw blocks like scalar, post processing ) and expose them as a single 
plane so that user space
compositor ( drm-hwc or weston) need not worry about max pixel width 
supported by the planes. But mapping
planes <-> hwpipe dynamically may need more than just virtualization. 
Kernel need to keep track of hwpipes
especially when dealing with multiple displays. And it get complicated 
when planes start moving around CRTC's

for different sized buffers.

Given that DRM exposes planes with its own list of format/modifiers, I 
think its not that big of a deal for a

compositor to 

Re: [RFC PULL] Add Display Support for Qualcomm SDM845

[Rob Clark]

On Tue, Feb 13, 2018 at 3:00 PM, Rob Clark  wrote:
> On Tue, Feb 13, 2018 at 2:18 PM, Sean Paul  wrote:
>> Hi dri-devel,
>> Qualcomm has been working for the past few weeks on forward porting their
>> downstream drm driver from 4.14 to mainline. Please consider this PR as a
>> request for review, rather than an attempt at mainlining the code as it
>> currently stands. The goal is get this driver in shape over the next coming
>> months.
>>
>> In the meantime, I'll be hosting a tree here [1] to stage the fixes. Patches
>> will be posted and reviewed on linux-arm-...@vger.kernel.org. Once things 
>> look
>> good, I'll send another pull 4realz.
>>
>> To get the ball rolling, I've done some review on the new connector code, my
>> comments are below.
>>
>> Thanks in advance for your constructive feedback :)
>>
>> Sean
>>
>> [1]- git://people.freedesktop.org/~seanpaul/dpu-staging
>>
>> Review feedback:
>> 
>
> just so others aren't confused, s/sde/dpu/g in the list below (we did
> our DAL->DC rename before sending first RFC :-P)
>
>> - Solve the splash screen handling (or remove it)
>> - Simplify devicetree binding (remove register offsets)
>> feedback from reviewing sde_connector.c:
>> - Rationalize backlight implementation in sde_connector (display_count 
>> static)
>> - Sort out the dsi event passing between dsi/encoder/connector (move to 
>> encoder)
>> - include/uapi/drm/msm_drm_pp.h needs opensource userspace (or removal)
>> - connector->state access violations reading/writing mode_info
>> - s/sde_rect/drm_rect/
>> - sde_kms_info usage needs to be replaced with formal data structures (not
>>   stringified keypairs)
>> - sde_connector_ops needs to be trimmed, duplicates connector helpers, info
>>   hooks circumvent state, and other hooks should be stored in state or
>>   prepopulated (get_dst_format)
>> - sde_connector_get_dpms unused
>> - esd status check should migrate to encoder from connector
>> - backlight should be handled in panel drivers, not in the generic 
>> connector/dsi
>>   encoder
>> - sde_connector_helper_bridge_disable is called from encoder and calls back 
>> into
>>   set_power encoder function. if backlight, and esd status are removed,
>>   pre_kickoff can probably go away
>> - sde_connector_clk_ctrl is another example of encoder->connector->encoder 
>> call
>> - RETIRE_FENCE connector property should be removed, opting for the native
>>   atomic fences
>> - ROI (regions of interest) should be expressed per-plane instead of 
>> connector.
>>   there is work ongoing to support dirty_rects per-plane by Deepak Singh 
>> Rawat
>>    and Lukasz Spintzyk 
>> - Uma Shankar  has proposed HDR source metadata
>>   properties on the list, we should pivot to those instead of hand-rolling 
>> them
>>   in the sde driver
>> - Convert HDCP implementation to upstream Content Protection property
>> - Merge dsi and dsi_staging into one driver
>> - Writeback connector has been proposed by ARM (Liviu Dudau and Brian 
>> Starkey),
>>   we should work with their proposal instead of rolling OUT_FB ourselves
>> - sde_connector_set_property should be replaced with atomic helper
>> - dsi hotplug can probably be punted to the panel driver
>> - dpms should switch to enable/disable (or at least use the atomic helpers)
>> - dsi mode handling should also defer to the panel driver
>> - SDE_WB_CONFIG ioctl should be removed in favor of the existing ioctl to add
>>   user-defined modes
>> - dp implementation should use the existing dp helpers wherever possible
>> - lots of duplicated structures in dsi_defs.h that can be replaced with 
>> existing
>>   drm structs
>> - mode_valid should be split up and implemented directly in 
>> connector/encoder as
>>   appropriate
>> - sde_connector->aspace seems like it's unused?
>>
>
> To add to that, a few things I've noticed (but I've mostly only gotten
> as far as the front-end of the pipeline, ie. dpu_plane):
>
>  - It looks like, as was the case with mdp4/mdp5 (and really most other hw)
>there are still unequal capabilities for planes (ie. some can do YUV,
>scaling, etc).
>
>But drm-hwc (or weston) isn't going to know about that, so I think we'll
>need to add support for dynamically assigning dpu_plane::pipe, similar
>to what mdp5 does w/ plane<->hwpipe.  (Which I actually added specifically
>for drm-hwc ;-))
>
>  - I *think* we also need the same trick for combining mixers under one CRTC
>for 4k modes too?
>
>  - in dpu_plane, and perhaps elsewhere, userspace pointers for things like CSC
>and scaling coefficients need to be annotated w/ __user.  (Except the 
> should
>probably be blob properties instead.. and we probably need to strip out the
>custom properties and re-introduce them separately with some userspace +
>review.)
>
>  - dpu_formats.c looks mostly like a superset of mdp_format.c, ie there is 

Re: [RFC PULL] Add Display Support for Qualcomm SDM845

[Daniel Stone]

Hi,

On 14 February 2018 at 13:50, Sean Paul  wrote:
> On Wed, Feb 14, 2018 at 07:22:53AM -0500, Rob Clark wrote:
>> On Wed, Feb 14, 2018 at 3:30 AM, Daniel Stone  wrote:
>> > This one I guess you can't get around though. Can they still run from
>> > the one plane, or do you secretly consume two planes?
>>
>> I think it is still the case, like mdp5, that you need two hw pipes..
>> but actually it gets more crazy, since there are some cases where two
>> planes could share a hw pipe.
>
> Right. Large fbs might require 2 pipes, and multiple overlapping or adjacent 
> small fbs
> can be serviced with 1 pipe.

I didn't know about using a single pipe for multiple framebuffers.
That's quite special indeed.

I think virtualising probably makes sense in that case: rather than
just getting around limitations, it's seeming like more of a VC4
situation where they just really aren't actually planes in the first
place.

Cheers,
Daniel
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC PULL] Add Display Support for Qualcomm SDM845

[Rob Clark]

On Wed, Feb 14, 2018 at 3:30 AM, Daniel Stone  wrote:
> Hi Rob,
>
> On 13 February 2018 at 20:00, Rob Clark  wrote:
>> On Tue, Feb 13, 2018 at 2:18 PM, Sean Paul  wrote:
>>> Qualcomm has been working for the past few weeks on forward porting their
>>> downstream drm driver from 4.14 to mainline. Please consider this PR as a
>>> request for review, rather than an attempt at mainlining the code as it
>>> currently stands. The goal is get this driver in shape over the next coming
>>> months.
>>
>> just so others aren't confused, s/sde/dpu/g in the list below (we did
>> our DAL->DC rename before sending first RFC :-P)
>
> Heh, and it is quite a bit of code to dig through. I haven't yet got a
> chance to fully check it out.
>
>>> - include/uapi/drm/msm_drm_pp.h needs opensource userspace (or removal)
>
> We don't really have a good story for pixel-processing API anywhere tbh.
>
>> To add to that, a few things I've noticed (but I've mostly only gotten
>> as far as the front-end of the pipeline, ie. dpu_plane):
>>
>>  - It looks like, as was the case with mdp4/mdp5 (and really most other hw)
>>there are still unequal capabilities for planes (ie. some can do YUV,
>>scaling, etc).
>>
>>But drm-hwc (or weston) isn't going to know about that, so I think we'll
>>need to add support for dynamically assigning dpu_plane::pipe, similar
>>to what mdp5 does w/ plane<->hwpipe.  (Which I actually added specifically
>>for drm-hwc ;-))
>
> Formats/modifiers we do at least get per-plane. We won't know about
> scaling, but at least Weston will go through trying each plane in
> sequence until it finds one which accepts the configuration. Could HWC
> do something like that with atomic, or does it rely on coming up with
> a plan first rather than the brute-force testing approach? I haven't
> seen its planner at all recently I'm afraid.
>
>>  - I *think* we also need the same trick for combining mixers under one CRTC
>>for 4k modes too?
>
> This one I guess you can't get around though. Can they still run from
> the one plane, or do you secretly consume two planes?
>

I think it is still the case, like mdp5, that you need two hw pipes..
but actually it gets more crazy, since there are some cases where two
planes could share a hw pipe.  Not sure that weston or drm-hwc is
going to have much hope in being able to deal with that, so I think
virtualizing the planes and crtcs is the better route.

BR,
-R

>>  - in dpu_plane, and perhaps elsewhere, userspace pointers for things like 
>> CSC
>>and scaling coefficients need to be annotated w/ __user.  (Except the 
>> should
>>probably be blob properties instead.. and we probably need to strip out 
>> the
>>custom properties and re-introduce them separately with some userspace +
>>review.)
>
> It'd be good to know if the CSC could use the CRTC GAMMA_LUT -> CTM ->
> DEGAMMA_LUT properties, if they were moved over to planes. (And if
> not, why not; if there's any functionality missing from those, what it
> is, etc.)
>
> Cheers,
> Daniel
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC PULL] Add Display Support for Qualcomm SDM845

[Daniel Stone]

Hi Rob,

On 13 February 2018 at 20:00, Rob Clark  wrote:
> On Tue, Feb 13, 2018 at 2:18 PM, Sean Paul  wrote:
>> Qualcomm has been working for the past few weeks on forward porting their
>> downstream drm driver from 4.14 to mainline. Please consider this PR as a
>> request for review, rather than an attempt at mainlining the code as it
>> currently stands. The goal is get this driver in shape over the next coming
>> months.
>
> just so others aren't confused, s/sde/dpu/g in the list below (we did
> our DAL->DC rename before sending first RFC :-P)

Heh, and it is quite a bit of code to dig through. I haven't yet got a
chance to fully check it out.

>> - include/uapi/drm/msm_drm_pp.h needs opensource userspace (or removal)

We don't really have a good story for pixel-processing API anywhere tbh.

> To add to that, a few things I've noticed (but I've mostly only gotten
> as far as the front-end of the pipeline, ie. dpu_plane):
>
>  - It looks like, as was the case with mdp4/mdp5 (and really most other hw)
>there are still unequal capabilities for planes (ie. some can do YUV,
>scaling, etc).
>
>But drm-hwc (or weston) isn't going to know about that, so I think we'll
>need to add support for dynamically assigning dpu_plane::pipe, similar
>to what mdp5 does w/ plane<->hwpipe.  (Which I actually added specifically
>for drm-hwc ;-))

Formats/modifiers we do at least get per-plane. We won't know about
scaling, but at least Weston will go through trying each plane in
sequence until it finds one which accepts the configuration. Could HWC
do something like that with atomic, or does it rely on coming up with
a plan first rather than the brute-force testing approach? I haven't
seen its planner at all recently I'm afraid.

>  - I *think* we also need the same trick for combining mixers under one CRTC
>for 4k modes too?

This one I guess you can't get around though. Can they still run from
the one plane, or do you secretly consume two planes?

>  - in dpu_plane, and perhaps elsewhere, userspace pointers for things like CSC
>and scaling coefficients need to be annotated w/ __user.  (Except the 
> should
>probably be blob properties instead.. and we probably need to strip out the
>custom properties and re-introduce them separately with some userspace +
>review.)

It'd be good to know if the CSC could use the CRTC GAMMA_LUT -> CTM ->
DEGAMMA_LUT properties, if they were moved over to planes. (And if
not, why not; if there's any functionality missing from those, what it
is, etc.)

Cheers,
Daniel
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC PULL] Add Display Support for Qualcomm SDM845

[Rob Clark]

On Tue, Feb 13, 2018 at 2:18 PM, Sean Paul  wrote:
> Hi dri-devel,
> Qualcomm has been working for the past few weeks on forward porting their
> downstream drm driver from 4.14 to mainline. Please consider this PR as a
> request for review, rather than an attempt at mainlining the code as it
> currently stands. The goal is get this driver in shape over the next coming
> months.
>
> In the meantime, I'll be hosting a tree here [1] to stage the fixes. Patches
> will be posted and reviewed on linux-arm-...@vger.kernel.org. Once things look
> good, I'll send another pull 4realz.
>
> To get the ball rolling, I've done some review on the new connector code, my
> comments are below.
>
> Thanks in advance for your constructive feedback :)
>
> Sean
>
> [1]- git://people.freedesktop.org/~seanpaul/dpu-staging
>
> Review feedback:
> 

just so others aren't confused, s/sde/dpu/g in the list below (we did
our DAL->DC rename before sending first RFC :-P)

> - Solve the splash screen handling (or remove it)
> - Simplify devicetree binding (remove register offsets)
> feedback from reviewing sde_connector.c:
> - Rationalize backlight implementation in sde_connector (display_count static)
> - Sort out the dsi event passing between dsi/encoder/connector (move to 
> encoder)
> - include/uapi/drm/msm_drm_pp.h needs opensource userspace (or removal)
> - connector->state access violations reading/writing mode_info
> - s/sde_rect/drm_rect/
> - sde_kms_info usage needs to be replaced with formal data structures (not
>   stringified keypairs)
> - sde_connector_ops needs to be trimmed, duplicates connector helpers, info
>   hooks circumvent state, and other hooks should be stored in state or
>   prepopulated (get_dst_format)
> - sde_connector_get_dpms unused
> - esd status check should migrate to encoder from connector
> - backlight should be handled in panel drivers, not in the generic 
> connector/dsi
>   encoder
> - sde_connector_helper_bridge_disable is called from encoder and calls back 
> into
>   set_power encoder function. if backlight, and esd status are removed,
>   pre_kickoff can probably go away
> - sde_connector_clk_ctrl is another example of encoder->connector->encoder 
> call
> - RETIRE_FENCE connector property should be removed, opting for the native
>   atomic fences
> - ROI (regions of interest) should be expressed per-plane instead of 
> connector.
>   there is work ongoing to support dirty_rects per-plane by Deepak Singh Rawat
>    and Lukasz Spintzyk 
> - Uma Shankar  has proposed HDR source metadata
>   properties on the list, we should pivot to those instead of hand-rolling 
> them
>   in the sde driver
> - Convert HDCP implementation to upstream Content Protection property
> - Merge dsi and dsi_staging into one driver
> - Writeback connector has been proposed by ARM (Liviu Dudau and Brian 
> Starkey),
>   we should work with their proposal instead of rolling OUT_FB ourselves
> - sde_connector_set_property should be replaced with atomic helper
> - dsi hotplug can probably be punted to the panel driver
> - dpms should switch to enable/disable (or at least use the atomic helpers)
> - dsi mode handling should also defer to the panel driver
> - SDE_WB_CONFIG ioctl should be removed in favor of the existing ioctl to add
>   user-defined modes
> - dp implementation should use the existing dp helpers wherever possible
> - lots of duplicated structures in dsi_defs.h that can be replaced with 
> existing
>   drm structs
> - mode_valid should be split up and implemented directly in connector/encoder 
> as
>   appropriate
> - sde_connector->aspace seems like it's unused?
>

To add to that, a few things I've noticed (but I've mostly only gotten
as far as the front-end of the pipeline, ie. dpu_plane):

 - It looks like, as was the case with mdp4/mdp5 (and really most other hw)
   there are still unequal capabilities for planes (ie. some can do YUV,
   scaling, etc).

   But drm-hwc (or weston) isn't going to know about that, so I think we'll
   need to add support for dynamically assigning dpu_plane::pipe, similar
   to what mdp5 does w/ plane<->hwpipe.  (Which I actually added specifically
   for drm-hwc ;-))

 - I *think* we also need the same trick for combining mixers under one CRTC
   for 4k modes too?

 - in dpu_plane, and perhaps elsewhere, userspace pointers for things like CSC
   and scaling coefficients need to be annotated w/ __user.  (Except the should
   probably be blob properties instead.. and we probably need to strip out the
   custom properties and re-introduce them separately with some userspace +
   review.)

 - dpu_formats.c looks mostly like a superset of mdp_format.c, ie there is a
   similar concept to how you describe the format to hw as mdp4/mdp5.  Probably
   the additions in dpu_formats should be folded back in mdp_format.

I'm not sure how we want to balance adding new features