Re: [Freedreno] [RFC PULL] Add Display Support for Qualcomm SDM845
On Wed, Feb 14, 2018 at 2:08 PM, Rob Clarkwrote: > 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 ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [RFC PULL] Add Display Support for Qualcomm SDM845
On Wed, Feb 14, 2018 at 1:17 PM, jsankawrote: > > > 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
Re: [Freedreno] [RFC PULL] Add Display Support for Qualcomm SDM845
On 2/13/2018 12:00 PM, Rob Clark wrote: On Tue, Feb 13, 2018 at 2:18 PM, Sean Paulwrote: 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: [Freedreno] [RFC PULL] Add Display Support for Qualcomm SDM845
On Tue, Feb 13, 2018 at 3:00 PM, Rob Clarkwrote: > 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: [Freedreno] [RFC PULL] Add Display Support for Qualcomm SDM845
Hi, On 14 February 2018 at 13:50, Sean Paulwrote: > 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 ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [RFC PULL] Add Display Support for Qualcomm SDM845
On Wed, Feb 14, 2018 at 07:22:53AM -0500, Rob Clark wrote: > On Wed, Feb 14, 2018 at 3:30 AM, Daniel Stonewrote: > > 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. It could definitely build up a plan a la weston, it just doesn't atm. > > > >> - 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. Right. Large fbs might require 2 pipes, and multiple overlapping or adjacent small fbs can be serviced with 1 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. Agreed. I think kernel is in the best place to make these decisions. Sean > > 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 -- Sean Paul, Software Engineer, Google / Chromium OS ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [RFC PULL] Add Display Support for Qualcomm SDM845
On Wed, Feb 14, 2018 at 3:30 AM, Daniel Stonewrote: > 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 ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [RFC PULL] Add Display Support for Qualcomm SDM845
Hi Rob, On 13 February 2018 at 20:00, Rob Clarkwrote: > 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 ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [RFC PULL] Add Display Support for Qualcomm SDM845
On Tue, Feb 13, 2018 at 7:02 PM, Jordan Crousewrote: > On Tue, Feb 13, 2018 at 02:18:13PM -0500, 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. >> >> drivers/gpu/drm/msm/msm_rd.c | 58 +- > > These changes are somewhat overzealous pointer verification, If > struct file * and/or struct inode * are NULL in a fops function your system is > already having a bad day. The only check that might be somewhat reasonable is > for gpu->funcs->get_param but that hook is defined on all targets so I don't > think it is unreasonable to assume that it should be there (and if it isn't > the > unit test sure better catch it). We can safely drop these changes. > thanks, this was something I didn't notice (or test) yet.. a couple of the other 'elfring fixes' I encountered (I guess there are probably some coding standards or static analysis behind this?) actually broke things and I have already reverted in the course of unbreaking display/gpu on db410c.. But yeah, ->get_param() is mandatory in the ioctl path (which is something you don't need debugfs access for).. for the paranoid probably the better thing would be a WARN_ON(!gpu->get_param) in gpu init path. BR, -R ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [RFC PULL] Add Display Support for Qualcomm SDM845
On Tue, Feb 13, 2018 at 02:18:13PM -0500, 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. > > drivers/gpu/drm/msm/msm_rd.c | 58 +- These changes are somewhat overzealous pointer verification, If struct file * and/or struct inode * are NULL in a fops function your system is already having a bad day. The only check that might be somewhat reasonable is for gpu->funcs->get_param but that hook is defined on all targets so I don't think it is unreasonable to assume that it should be there (and if it isn't the unit test sure better catch it). We can safely drop these changes. Jordan -- The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [RFC PULL] Add Display Support for Qualcomm SDM845
On Tue, Feb 13, 2018 at 2:18 PM, Sean Paulwrote: > 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