Re: [Freedreno] [PATCH v4 1/4] drm/msm/dpu: Move TE setup to prepare_for_kickoff()
On 01/03/2023 19:08, Marijn Suijten wrote: On 2023-03-01 08:23:28, Abhinav Kumar wrote: On 3/1/2023 2:03 AM, Marijn Suijten wrote: On 2023-02-21 10:42:53, Jessica Zhang wrote: Then, for some patch hygiene, starting here: Depends on: "Implement tearcheck support on INTF block" [3] Changes in V3: - Added function prototypes - Reordered function definitions to make change more legible - Removed prepare_commit() function from dpu_encoder_phys_cmd Changes in V4: - Reworded commit message to be more specific - Removed dpu_encoder_phys_cmd_is_ongoing_pptx() prototype ... until here: all this info belongs /below the cut/ outside of the messge that becomes part of the commit when this patch is applied to the tree. For DRM, I thought we are keeping the change log above the ? Which means its allowed in the commit message. I hope not, seems unlikely to have different rules across kernel subsystems. The main point is that this changelog and dependency chain isn't of any value when the final patch is applied, regardless of whether it is "allowed". Unfortunately this is one of DRM peculiarities. So, some of the patches have changelog in commit message. I myself prefer to have changelog in the cover letter, but I don't enforce that. -- With best wishes Dmitry
Re: [Freedreno] [PATCH v4 1/4] drm/msm/dpu: Move TE setup to prepare_for_kickoff()
On 3/1/2023 9:08 AM, Marijn Suijten wrote: On 2023-03-01 08:23:28, Abhinav Kumar wrote: On 3/1/2023 2:03 AM, Marijn Suijten wrote: On 2023-02-21 10:42:53, Jessica Zhang wrote: Currently, DPU will enable TE during prepare_commit(). However, this will cause a crash and reboot to sahara when trying to read/write to register in get_autorefresh_config(), because the core clock rates aren't set at that time. Haven't seeen a crash like this on any of my devices (after implementing INTF TE). get_autorefresh_config() always reads zero (or 1 for frame_count) except the first time it is called (autorefresh is left enabled by our bootloader on SM6125) and triggers the disable codepath. I feel that the fact that bootloader keeps things on for you is the reason you dont see the issue. With continuoush splash, clocks are kept enabled. We dont have it enabled (confirmed that). That is quite likely, we may even have them enabled because of simple-framebuffer in DTs; turning those off likely won't have any effect for testing this. For what it's worth, my SM8150 reads 0 for autorefresh. the value shouldnt really matter. The fact that you are able to read that register without crashing like we are means your clocks are on and ours arent. Thats what this change is fixing. Then, for some patch hygiene, starting here: Depends on: "Implement tearcheck support on INTF block" [3] Changes in V3: - Added function prototypes - Reordered function definitions to make change more legible - Removed prepare_commit() function from dpu_encoder_phys_cmd Changes in V4: - Reworded commit message to be more specific - Removed dpu_encoder_phys_cmd_is_ongoing_pptx() prototype ... until here: all this info belongs /below the cut/ outside of the messge that becomes part of the commit when this patch is applied to the tree. For DRM, I thought we are keeping the change log above the ? Which means its allowed in the commit message. I hope not, seems unlikely to have different rules across kernel subsystems. The main point is that this changelog and dependency chain isn't of any value when the final patch is applied, regardless of whether it is "allowed". I looked at a recently posted change by Rob and change log is above the --- https://patchwork.kernel.org/project/dri-devel/patch/20230301185432.3010939-1-robdcl...@gmail.com/ So we will follow that. [1] https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c#L1109 [2] https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c#L2339 Please replace these with "permalinks" (to a commit hash): a branch with line number annotation will fall out of date soon as more patches are applied that touch these files. [3] https://patchwork.freedesktop.org/series/112332/ Is this a hard dependency? It seems this series applies cleanly on -next and - from a cursory view - should be applicable and testable without my INTF TE series. However, Dmitry asked me to move some code around in review resulting in separate callbacks in the encoder, rather than having various if(has_intf_te) within those callbacks. That'll cause conflicts when I eventually get to respin a v2. I guess Jessica listed this because without intf_te series there is no crash because hw_pp would be NULL and autorefresh() would return early. So dependency is from the standpoint of when this series is needed and not from compilation point of view. That is indeed the question. I'll leave it to the maintainers to decide what order to apply these in, which we should be made aware of before submitting v2 so that one of us can resolve the conflicts. It should be first the intf TE series and then this one. You can go ahead and post your v2, we will rebase on top of yours. - Marijn
Re: [Freedreno] [PATCH v3] drm/msm/dp: check core_initialized flag at both host_init() and host_deinit()
On 01/03/2023 18:57, Kuogee Hsieh wrote:
On 2/28/2023 6:16 PM, Dmitry Baryshkov wrote:
On Wed, 1 Mar 2023 at 02:17, Kuogee Hsieh
wrote:
There is a reboot/suspend test case where system suspend is forced
during system booting up. Since dp_display_host_init() of external
DP is executed at hpd thread context, this test case may created a
scenario that dp_display_host_deinit() from pm_suspend() run before
dp_display_host_init() if hpd thread has no chance to run during
booting up while suspend request command was issued. At this scenario
system will crash at aux register access at dp_display_host_deinit()
since aux clock had not yet been enabled by dp_display_host_init().
Therefore we have to ensure aux clock enabled by checking
core_initialized flag before access aux registers at pm_suspend.
Can a call to dp_display_host_init() be moved from
dp_display_config_hpd() to dp_display_bind()?
yes, Sankeerth's "drm/msm/dp: enable pm_runtime support for dp driver"
patch is doing that which is under review.
https://patchwork.freedesktop.org/patch/523879/?series=114297=1
No, he is doing another thing. He is moving these calls to pm_runtime
callbacks, not to the dp_display_bind().
Related question: what is the primary reason for having
EV_HPD_INIT_SETUP and calling dp_display_config_hpd() via the event
thread? Does DP driver really depend on DPU irqs being installed? As
far as I understand, DP device uses MDSS interrupts and those IRQs are
available and working at the time of dp_display_probe() /
dp_display_bind().
HDP gpio pin has to run through DP aux module 100ms denouncing logic and
have its mask bits.
Therefore DP irq has to be enabled to receive DP isr with mask bits set.
So... DP irq is enabled by the MDSS, not by the DPU. Again, why does DP
driver depend on DPU irqs being installed?
Similar mechanism is used for mdp, dsi, etc.
And none of them uses irq_postinstall callback.
Changes in v2:
-- at commit text, dp_display_host_init() instead of host_init()
-- at commit text, dp_display_host_deinit() instead of host_deinit()
Changes in v3:
-- re arrange to avoid commit text line over 75 chars
Fixes: 989ebe7bc446 ("drm/msm/dp: do not initialize phy until plugin
interrupt received")
Signed-off-by: Kuogee Hsieh
Reviewed-by: Stephen Boyd
---
drivers/gpu/drm/msm/dp/dp_display.c | 20
1 file changed, 12 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
b/drivers/gpu/drm/msm/dp/dp_display.c
index bde1a7c..1850738 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -460,10 +460,12 @@ static void dp_display_host_init(struct
dp_display_private *dp)
dp->dp_display.connector_type, dp->core_initialized,
dp->phy_initialized);
- dp_power_init(dp->power, false);
- dp_ctrl_reset_irq_ctrl(dp->ctrl, true);
- dp_aux_init(dp->aux);
- dp->core_initialized = true;
+ if (!dp->core_initialized) {
+ dp_power_init(dp->power, false);
+ dp_ctrl_reset_irq_ctrl(dp->ctrl, true);
+ dp_aux_init(dp->aux);
+ dp->core_initialized = true;
+ }
}
static void dp_display_host_deinit(struct dp_display_private *dp)
@@ -472,10 +474,12 @@ static void dp_display_host_deinit(struct
dp_display_private *dp)
dp->dp_display.connector_type, dp->core_initialized,
dp->phy_initialized);
- dp_ctrl_reset_irq_ctrl(dp->ctrl, false);
- dp_aux_deinit(dp->aux);
- dp_power_deinit(dp->power);
- dp->core_initialized = false;
+ if (dp->core_initialized) {
+ dp_ctrl_reset_irq_ctrl(dp->ctrl, false);
+ dp_aux_deinit(dp->aux);
+ dp_power_deinit(dp->power);
+ dp->core_initialized = false;
+ }
}
static int dp_display_usbpd_configure_cb(struct device *dev)
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
Forum,
a Linux Foundation Collaborative Project
--
With best wishes
Dmitry
Re: [Freedreno] [PATCH v13 00/13] Add PSR support for eDP
On 01/03/2023 21:06, Doug Anderson wrote: Hi, On Sun, Feb 12, 2023 at 8:29 AM Vinod Polimera wrote: Changes in v2: - Use dp bridge to set psr entry/exit instead of dpu_enocder. - Don't modify whitespaces. - Set self refresh aware from atomic_check. - Set self refresh aware only if psr is supported. - Provide a stub for msm_dp_display_set_psr. - Move dp functions to bridge code. Changes in v3: - Change callback names to reflect atomic interfaces. - Move bridge callback change to separate patch as suggested by Dmitry. - Remove psr function declaration from msm_drv.h. - Set self_refresh_aware flag only if psr is supported. - Modify the variable names to simpler form. - Define bit fields for PSR settings. - Add comments explaining the steps to enter/exit psr. - Change DRM_INFO to drm_dbg_db. Changes in v4: - Move the get crtc functions to drm_atomic. - Add atomic functions for DP bridge too. - Add ternary operator to choose eDP or DP ops. - Return true/false instead of 1/0. - mode_valid missing in the eDP bridge ops. - Move the functions to get crtc into drm_atomic.c. - Fix compilation issues. - Remove dpu_assign_crtc and get crtc from drm_enc instead of dpu_enc. - Check for crtc state enable while reserving resources. Changes in v5: - Move the mode_valid changes into a different patch. - Complete psr_op_comp only when isr is set. - Move the DP atomic callback changes to a different patch. - Get crtc from drm connector state crtc. - Move to separate patch for check for crtc state enable while reserving resources. Changes in v6: - Remove crtc from dpu_encoder_virt struct. - fix crtc check during vblank toggle crtc. - Misc changes. Changes in v7: - Add fix for underrun issue on kasan build. Changes in v8: - Drop the enc spinlock as it won't serve any purpose in protetcing conn state.(Dmitry/Doug) Changes in v9: - Update commit message and fix alignment using spaces.(Marijn) - Misc changes.(Marijn) Changes in v10: - Get crtc cached in dpu_enc during obj init.(Dmitry) Changes in v11: - Remove crtc cached in dpu_enc during obj init. - Update dpu_enc crtc state on crtc enable/disable during self refresh. Changes in v12: - Update sc7180 intf mask to get intf timing gen status based on DPU_INTF_STATUS_SUPPORTED bit.(Dmitry) - Remove "clear active interface in the datapath cleanup" change as it is already included. Changes in v13: - Move core changes to top of the series.(Dmitry) - Drop self refresh aware disable change after psr entry.(Dmitry) Vinod Polimera (13): drm: add helper functions to retrieve old and new crtc drm/bridge: use atomic enable/disable callbacks for panel bridge drm/bridge: add psr support for panel bridge callbacks drm/msm/disp/dpu: check for crtc enable rather than crtc active to release shared resources drm/msm/disp/dpu: get timing engine status from intf status register drm/msm/disp/dpu: wait for extra vsync till timing engine status is disabled drm/msm/disp/dpu: reset the datapath after timing engine disable drm/msm/dp: use atomic callbacks for DP bridge ops drm/msm/dp: Add basic PSR support for eDP drm/msm/dp: use the eDP bridge ops to validate eDP modes drm/msm/disp/dpu: use atomic enable/disable callbacks for encoder functions drm/msm/disp/dpu: add PSR support for eDP interface in dpu driver drm/msm/disp/dpu: update dpu_enc crtc state on crtc enable/disable during self refresh I'm curious what the plan is for landing this series. I could land the first two in drm-misc if you want, but I'm a lowly committer and so I couldn't make an immutable branch for you nor can I officially Ack the changes to land in your branch. That means you'd be blocked for an extra version. Do you already have a plan? If not, then maybe we need to get in touch with one of the maintainers [1] of drm-misc? That's documented [2] to be in their set of responsibilities. [1] https://drm.pages.freedesktop.org/maintainer-tools/repositories.html#drm-misc-repository [2] https://drm.pages.freedesktop.org/maintainer-tools/maintainer-drm-misc.html#maintainer-s-duties My plan was to ask Thomas or Daniel to make an immutable branch. I just didn't want to ping them before rc1. -Doug -- With best wishes Dmitry
Re: [Freedreno] [PATCH v13 00/13] Add PSR support for eDP
Hi, On Wed, Mar 1, 2023 at 11:06 AM Doug Anderson wrote: > > Hi, > > On Sun, Feb 12, 2023 at 8:29 AM Vinod Polimera > wrote: > > > > Changes in v2: > > - Use dp bridge to set psr entry/exit instead of dpu_enocder. > > - Don't modify whitespaces. > > - Set self refresh aware from atomic_check. > > - Set self refresh aware only if psr is supported. > > - Provide a stub for msm_dp_display_set_psr. > > - Move dp functions to bridge code. > > > > Changes in v3: > > - Change callback names to reflect atomic interfaces. > > - Move bridge callback change to separate patch as suggested by Dmitry. > > - Remove psr function declaration from msm_drv.h. > > - Set self_refresh_aware flag only if psr is supported. > > - Modify the variable names to simpler form. > > - Define bit fields for PSR settings. > > - Add comments explaining the steps to enter/exit psr. > > - Change DRM_INFO to drm_dbg_db. > > > > Changes in v4: > > - Move the get crtc functions to drm_atomic. > > - Add atomic functions for DP bridge too. > > - Add ternary operator to choose eDP or DP ops. > > - Return true/false instead of 1/0. > > - mode_valid missing in the eDP bridge ops. > > - Move the functions to get crtc into drm_atomic.c. > > - Fix compilation issues. > > - Remove dpu_assign_crtc and get crtc from drm_enc instead of dpu_enc. > > - Check for crtc state enable while reserving resources. > > > > Changes in v5: > > - Move the mode_valid changes into a different patch. > > - Complete psr_op_comp only when isr is set. > > - Move the DP atomic callback changes to a different patch. > > - Get crtc from drm connector state crtc. > > - Move to separate patch for check for crtc state enable while > > reserving resources. > > > > Changes in v6: > > - Remove crtc from dpu_encoder_virt struct. > > - fix crtc check during vblank toggle crtc. > > - Misc changes. > > > > Changes in v7: > > - Add fix for underrun issue on kasan build. > > > > Changes in v8: > > - Drop the enc spinlock as it won't serve any purpose in > > protetcing conn state.(Dmitry/Doug) > > > > Changes in v9: > > - Update commit message and fix alignment using spaces.(Marijn) > > - Misc changes.(Marijn) > > > > Changes in v10: > > - Get crtc cached in dpu_enc during obj init.(Dmitry) > > > > Changes in v11: > > - Remove crtc cached in dpu_enc during obj init. > > - Update dpu_enc crtc state on crtc enable/disable during self refresh. > > > > Changes in v12: > > - Update sc7180 intf mask to get intf timing gen status > > based on DPU_INTF_STATUS_SUPPORTED bit.(Dmitry) > > - Remove "clear active interface in the datapath cleanup" change > > as it is already included. > > > > Changes in v13: > > - Move core changes to top of the series.(Dmitry) > > - Drop self refresh aware disable change after psr entry.(Dmitry) > > > > Vinod Polimera (13): > > drm: add helper functions to retrieve old and new crtc > > drm/bridge: use atomic enable/disable callbacks for panel bridge > > drm/bridge: add psr support for panel bridge callbacks > > drm/msm/disp/dpu: check for crtc enable rather than crtc active to > > release shared resources > > drm/msm/disp/dpu: get timing engine status from intf status register > > drm/msm/disp/dpu: wait for extra vsync till timing engine status is > > disabled > > drm/msm/disp/dpu: reset the datapath after timing engine disable > > drm/msm/dp: use atomic callbacks for DP bridge ops > > drm/msm/dp: Add basic PSR support for eDP > > drm/msm/dp: use the eDP bridge ops to validate eDP modes > > drm/msm/disp/dpu: use atomic enable/disable callbacks for encoder > > functions > > drm/msm/disp/dpu: add PSR support for eDP interface in dpu driver > > drm/msm/disp/dpu: update dpu_enc crtc state on crtc enable/disable > > during self refresh > > I'm curious what the plan is for landing this series. I could land the > first two in drm-misc if you want, but I'm a lowly committer and so I > couldn't make an immutable branch for you nor can I officially Ack the > changes to land in your branch. That means you'd be blocked for an > extra version. Do you already have a plan? If not, then maybe we need > to get in touch with one of the maintainers [1] of drm-misc? That's > documented [2] to be in their set of responsibilities. > > [1] > https://drm.pages.freedesktop.org/maintainer-tools/repositories.html#drm-misc-repository > [2] > https://drm.pages.freedesktop.org/maintainer-tools/maintainer-drm-misc.html#maintainer-s-duties The above question about how we'd land this is still a good one, but perhaps less relevant quite yet because, at least in my testing, the current series doesn't work. :-/ I know previous versions worked, so I went back and tried older versions. It turns out that v12 _does_ work for me, but v13 doesn't work. The difference is very small. I'm assuming you made some changes in v13 and they looked so small that you just sent v13 out
Re: [Freedreno] [PATCH v13 00/13] Add PSR support for eDP
Hi, On Sun, Feb 12, 2023 at 8:29 AM Vinod Polimera wrote: > > Changes in v2: > - Use dp bridge to set psr entry/exit instead of dpu_enocder. > - Don't modify whitespaces. > - Set self refresh aware from atomic_check. > - Set self refresh aware only if psr is supported. > - Provide a stub for msm_dp_display_set_psr. > - Move dp functions to bridge code. > > Changes in v3: > - Change callback names to reflect atomic interfaces. > - Move bridge callback change to separate patch as suggested by Dmitry. > - Remove psr function declaration from msm_drv.h. > - Set self_refresh_aware flag only if psr is supported. > - Modify the variable names to simpler form. > - Define bit fields for PSR settings. > - Add comments explaining the steps to enter/exit psr. > - Change DRM_INFO to drm_dbg_db. > > Changes in v4: > - Move the get crtc functions to drm_atomic. > - Add atomic functions for DP bridge too. > - Add ternary operator to choose eDP or DP ops. > - Return true/false instead of 1/0. > - mode_valid missing in the eDP bridge ops. > - Move the functions to get crtc into drm_atomic.c. > - Fix compilation issues. > - Remove dpu_assign_crtc and get crtc from drm_enc instead of dpu_enc. > - Check for crtc state enable while reserving resources. > > Changes in v5: > - Move the mode_valid changes into a different patch. > - Complete psr_op_comp only when isr is set. > - Move the DP atomic callback changes to a different patch. > - Get crtc from drm connector state crtc. > - Move to separate patch for check for crtc state enable while > reserving resources. > > Changes in v6: > - Remove crtc from dpu_encoder_virt struct. > - fix crtc check during vblank toggle crtc. > - Misc changes. > > Changes in v7: > - Add fix for underrun issue on kasan build. > > Changes in v8: > - Drop the enc spinlock as it won't serve any purpose in > protetcing conn state.(Dmitry/Doug) > > Changes in v9: > - Update commit message and fix alignment using spaces.(Marijn) > - Misc changes.(Marijn) > > Changes in v10: > - Get crtc cached in dpu_enc during obj init.(Dmitry) > > Changes in v11: > - Remove crtc cached in dpu_enc during obj init. > - Update dpu_enc crtc state on crtc enable/disable during self refresh. > > Changes in v12: > - Update sc7180 intf mask to get intf timing gen status > based on DPU_INTF_STATUS_SUPPORTED bit.(Dmitry) > - Remove "clear active interface in the datapath cleanup" change > as it is already included. > > Changes in v13: > - Move core changes to top of the series.(Dmitry) > - Drop self refresh aware disable change after psr entry.(Dmitry) > > Vinod Polimera (13): > drm: add helper functions to retrieve old and new crtc > drm/bridge: use atomic enable/disable callbacks for panel bridge > drm/bridge: add psr support for panel bridge callbacks > drm/msm/disp/dpu: check for crtc enable rather than crtc active to > release shared resources > drm/msm/disp/dpu: get timing engine status from intf status register > drm/msm/disp/dpu: wait for extra vsync till timing engine status is > disabled > drm/msm/disp/dpu: reset the datapath after timing engine disable > drm/msm/dp: use atomic callbacks for DP bridge ops > drm/msm/dp: Add basic PSR support for eDP > drm/msm/dp: use the eDP bridge ops to validate eDP modes > drm/msm/disp/dpu: use atomic enable/disable callbacks for encoder > functions > drm/msm/disp/dpu: add PSR support for eDP interface in dpu driver > drm/msm/disp/dpu: update dpu_enc crtc state on crtc enable/disable > during self refresh I'm curious what the plan is for landing this series. I could land the first two in drm-misc if you want, but I'm a lowly committer and so I couldn't make an immutable branch for you nor can I officially Ack the changes to land in your branch. That means you'd be blocked for an extra version. Do you already have a plan? If not, then maybe we need to get in touch with one of the maintainers [1] of drm-misc? That's documented [2] to be in their set of responsibilities. [1] https://drm.pages.freedesktop.org/maintainer-tools/repositories.html#drm-misc-repository [2] https://drm.pages.freedesktop.org/maintainer-tools/maintainer-drm-misc.html#maintainer-s-duties -Doug
Re: [Freedreno] [PATCH v4 1/4] drm/msm/dpu: Move TE setup to prepare_for_kickoff()
On 2023-03-01 08:23:28, Abhinav Kumar wrote: > > On 3/1/2023 2:03 AM, Marijn Suijten wrote: > > On 2023-02-21 10:42:53, Jessica Zhang wrote: > >> Currently, DPU will enable TE during prepare_commit(). However, this > >> will cause a crash and reboot to sahara when trying to read/write to > >> register in get_autorefresh_config(), because the core clock rates > >> aren't set at that time. > > > > Haven't seeen a crash like this on any of my devices (after implementing > > INTF TE). get_autorefresh_config() always reads zero (or 1 for > > frame_count) except the first time it is called (autorefresh is left > > enabled by our bootloader on SM6125) and triggers the disable codepath. > > > > I feel that the fact that bootloader keeps things on for you is the > reason you dont see the issue. With continuoush splash, clocks are kept > enabled. We dont have it enabled (confirmed that). That is quite likely, we may even have them enabled because of simple-framebuffer in DTs; turning those off likely won't have any effect for testing this. For what it's worth, my SM8150 reads 0 for autorefresh. > > Then, for some patch hygiene, starting here: > > > >> Depends on: "Implement tearcheck support on INTF block" [3] > >> > >> Changes in V3: > >> - Added function prototypes > >> - Reordered function definitions to make change more legible > >> - Removed prepare_commit() function from dpu_encoder_phys_cmd > >> > >> Changes in V4: > >> - Reworded commit message to be more specific > >> - Removed dpu_encoder_phys_cmd_is_ongoing_pptx() prototype > > > > ... until here: all this info belongs /below the cut/ outside of the > > messge that becomes part of the commit when this patch is applied to the > > tree. > > For DRM, I thought we are keeping the change log above the ? > Which means its allowed in the commit message. I hope not, seems unlikely to have different rules across kernel subsystems. The main point is that this changelog and dependency chain isn't of any value when the final patch is applied, regardless of whether it is "allowed". > >> [1] > >> https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c#L1109 > >> [2] > >> https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c#L2339 > > > > Please replace these with "permalinks" (to a commit hash): a branch with > > line number annotation will fall out of date soon as more patches are > > applied that touch these files. > > > >> [3] https://patchwork.freedesktop.org/series/112332/ > > > > Is this a hard dependency? It seems this series applies cleanly on > > -next and - from a cursory view - should be applicable and testable > > without my INTF TE series. However, Dmitry asked me to move some code > > around in review resulting in separate callbacks in the encoder, rather > > than having various if(has_intf_te) within those callbacks. That'll > > cause conflicts when I eventually get to respin a v2. > > > > I guess Jessica listed this because without intf_te series there is no > crash because hw_pp would be NULL and autorefresh() would return early. > So dependency is from the standpoint of when this series is needed and > not from compilation point of view. That is indeed the question. I'll leave it to the maintainers to decide what order to apply these in, which we should be made aware of before submitting v2 so that one of us can resolve the conflicts. - Marijn
Re: [Freedreno] [PATCH v3] drm/msm/dp: check core_initialized flag at both host_init() and host_deinit()
On 2/28/2023 6:16 PM, Dmitry Baryshkov wrote:
On Wed, 1 Mar 2023 at 02:17, Kuogee Hsieh wrote:
There is a reboot/suspend test case where system suspend is forced
during system booting up. Since dp_display_host_init() of external
DP is executed at hpd thread context, this test case may created a
scenario that dp_display_host_deinit() from pm_suspend() run before
dp_display_host_init() if hpd thread has no chance to run during
booting up while suspend request command was issued. At this scenario
system will crash at aux register access at dp_display_host_deinit()
since aux clock had not yet been enabled by dp_display_host_init().
Therefore we have to ensure aux clock enabled by checking
core_initialized flag before access aux registers at pm_suspend.
Can a call to dp_display_host_init() be moved from
dp_display_config_hpd() to dp_display_bind()?
yes, Sankeerth's "drm/msm/dp: enable pm_runtime support for dp driver"
patch is doing that which is under review.
https://patchwork.freedesktop.org/patch/523879/?series=114297=1
Related question: what is the primary reason for having
EV_HPD_INIT_SETUP and calling dp_display_config_hpd() via the event
thread? Does DP driver really depend on DPU irqs being installed? As
far as I understand, DP device uses MDSS interrupts and those IRQs are
available and working at the time of dp_display_probe() /
dp_display_bind().
HDP gpio pin has to run through DP aux module 100ms denouncing logic and
have its mask bits.
Therefore DP irq has to be enabled to receive DP isr with mask bits set.
Similar mechanism is used for mdp, dsi, etc.
Changes in v2:
-- at commit text, dp_display_host_init() instead of host_init()
-- at commit text, dp_display_host_deinit() instead of host_deinit()
Changes in v3:
-- re arrange to avoid commit text line over 75 chars
Fixes: 989ebe7bc446 ("drm/msm/dp: do not initialize phy until plugin interrupt
received")
Signed-off-by: Kuogee Hsieh
Reviewed-by: Stephen Boyd
---
drivers/gpu/drm/msm/dp/dp_display.c | 20
1 file changed, 12 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
b/drivers/gpu/drm/msm/dp/dp_display.c
index bde1a7c..1850738 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -460,10 +460,12 @@ static void dp_display_host_init(struct
dp_display_private *dp)
dp->dp_display.connector_type, dp->core_initialized,
dp->phy_initialized);
- dp_power_init(dp->power, false);
- dp_ctrl_reset_irq_ctrl(dp->ctrl, true);
- dp_aux_init(dp->aux);
- dp->core_initialized = true;
+ if (!dp->core_initialized) {
+ dp_power_init(dp->power, false);
+ dp_ctrl_reset_irq_ctrl(dp->ctrl, true);
+ dp_aux_init(dp->aux);
+ dp->core_initialized = true;
+ }
}
static void dp_display_host_deinit(struct dp_display_private *dp)
@@ -472,10 +474,12 @@ static void dp_display_host_deinit(struct
dp_display_private *dp)
dp->dp_display.connector_type, dp->core_initialized,
dp->phy_initialized);
- dp_ctrl_reset_irq_ctrl(dp->ctrl, false);
- dp_aux_deinit(dp->aux);
- dp_power_deinit(dp->power);
- dp->core_initialized = false;
+ if (dp->core_initialized) {
+ dp_ctrl_reset_irq_ctrl(dp->ctrl, false);
+ dp_aux_deinit(dp->aux);
+ dp_power_deinit(dp->power);
+ dp->core_initialized = false;
+ }
}
static int dp_display_usbpd_configure_cb(struct device *dev)
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
Re: [Freedreno] [PATCH v4 1/4] drm/msm/dpu: Move TE setup to prepare_for_kickoff()
On 3/1/2023 2:03 AM, Marijn Suijten wrote:
On 2023-02-21 10:42:53, Jessica Zhang wrote:
Currently, DPU will enable TE during prepare_commit(). However, this
will cause a crash and reboot to sahara when trying to read/write to
register in get_autorefresh_config(), because the core clock rates
aren't set at that time.
Haven't seeen a crash like this on any of my devices (after implementing
INTF TE). get_autorefresh_config() always reads zero (or 1 for
frame_count) except the first time it is called (autorefresh is left
enabled by our bootloader on SM6125) and triggers the disable codepath.
I feel that the fact that bootloader keeps things on for you is the
reason you dont see the issue. With continuoush splash, clocks are kept
enabled. We dont have it enabled (confirmed that).
It does look like prepare_for_kickoff() is much more susceptible to
delays in code than prepare_commit(). The debug logs used to figure
this out together with this series result in FPS drops on SM6125 and
SM8150. Not an issue with this series, just something to keep in mind.
This used to work because phys_enc->hw_pp is only initialized in mode
set [1], so the first prepare_commit() will return before any register
read/write as hw_pp would be NULL.
However, when we try to implement support for INTF TE, we will run into
the clock issue described above as hw_intf will *not* be NULL on the
first prepare_commit(). This is because the initialization of
dpu_enc->hw_intf has been moved to dpu_encoder_setup() [2].
To avoid this issue, let's enable TE during prepare_for_kickoff()
instead as the core clock rates are guaranteed to be set then.
For the change itself:
Reviewed-by: Marijn Suijten
And tested on SM6125, SM8150 (INTF TE) and SDM845 (PP TE).
Thanks.
Then, for some patch hygiene, starting here:
Depends on: "Implement tearcheck support on INTF block" [3]
Changes in V3:
- Added function prototypes
- Reordered function definitions to make change more legible
- Removed prepare_commit() function from dpu_encoder_phys_cmd
Changes in V4:
- Reworded commit message to be more specific
- Removed dpu_encoder_phys_cmd_is_ongoing_pptx() prototype
... until here: all this info belongs /below the cut/ outside of the
messge that becomes part of the commit when this patch is applied to the
tree.
For DRM, I thought we are keeping the change log above the ?
Which means its allowed in the commit message.
[1]
https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c#L1109
[2]
https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c#L2339
Please replace these with "permalinks" (to a commit hash): a branch with
line number annotation will fall out of date soon as more patches are
applied that touch these files.
[3] https://patchwork.freedesktop.org/series/112332/
Is this a hard dependency? It seems this series applies cleanly on
-next and - from a cursory view - should be applicable and testable
without my INTF TE series. However, Dmitry asked me to move some code
around in review resulting in separate callbacks in the encoder, rather
than having various if(has_intf_te) within those callbacks. That'll
cause conflicts when I eventually get to respin a v2.
I guess Jessica listed this because without intf_te series there is no
crash because hw_pp would be NULL and autorefresh() would return early.
So dependency is from the standpoint of when this series is needed and
not from compilation point of view.
- Marijn
Signed-off-by: Jessica Zhang
---
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 8 +---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
index cb05036f2916..98958c75864a 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
@@ -40,6 +40,8 @@
#define DPU_ENC_MAX_POLL_TIMEOUT_US 2000
+static void dpu_encoder_phys_cmd_enable_te(struct dpu_encoder_phys *phys_enc);
+
static bool dpu_encoder_phys_cmd_is_master(struct dpu_encoder_phys *phys_enc)
{
return (phys_enc->split_role != ENC_ROLE_SLAVE);
@@ -611,6 +613,8 @@ static void dpu_encoder_phys_cmd_prepare_for_kickoff(
phys_enc->hw_pp->idx - PINGPONG_0);
}
+ dpu_encoder_phys_cmd_enable_te(phys_enc);
+
DPU_DEBUG_CMDENC(cmd_enc, "pp:%d pending_cnt %d\n",
phys_enc->hw_pp->idx - PINGPONG_0,
atomic_read(_enc->pending_kickoff_cnt));
@@ -641,8 +645,7 @@ static bool dpu_encoder_phys_cmd_is_ongoing_pptx(
return false;
}
-static void dpu_encoder_phys_cmd_prepare_commit(
- struct dpu_encoder_phys *phys_enc)
+static void dpu_encoder_phys_cmd_enable_te(struct dpu_encoder_phys *phys_enc)
{
struct dpu_encoder_phys_cmd
Re: [Freedreno] [PATCH v4 06/14] dma-buf/sync_file: Support (E)POLLPRI
On Wed, Mar 1, 2023 at 7:31 AM Sebastian Wick wrote: > > On Tue, Feb 28, 2023 at 11:52 PM Rob Clark wrote: > > > > On Tue, Feb 28, 2023 at 6:30 AM Sebastian Wick > > wrote: > > > > > > On Tue, Feb 28, 2023 at 12:48 AM Rob Clark wrote: > > > > > > > > On Mon, Feb 27, 2023 at 2:44 PM Sebastian Wick > > > > wrote: > > > > > > > > > > On Mon, Feb 27, 2023 at 11:20 PM Rob Clark > > > > > wrote: > > > > > > > > > > > > On Mon, Feb 27, 2023 at 1:36 PM Rodrigo Vivi > > > > > > wrote: > > > > > > > > > > > > > > On Fri, Feb 24, 2023 at 09:59:57AM -0800, Rob Clark wrote: > > > > > > > > On Fri, Feb 24, 2023 at 7:27 AM Luben Tuikov > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > On 2023-02-24 06:37, Tvrtko Ursulin wrote: > > > > > > > > > > > > > > > > > > > > On 24/02/2023 11:00, Pekka Paalanen wrote: > > > > > > > > > >> On Fri, 24 Feb 2023 10:50:51 + > > > > > > > > > >> Tvrtko Ursulin wrote: > > > > > > > > > >> > > > > > > > > > >>> On 24/02/2023 10:24, Pekka Paalanen wrote: > > > > > > > > > On Fri, 24 Feb 2023 09:41:46 + > > > > > > > > > Tvrtko Ursulin wrote: > > > > > > > > > > > > > > > > > > > On 24/02/2023 09:26, Pekka Paalanen wrote: > > > > > > > > > >> On Thu, 23 Feb 2023 10:51:48 -0800 > > > > > > > > > >> Rob Clark wrote: > > > > > > > > > >> > > > > > > > > > >>> On Thu, Feb 23, 2023 at 1:38 AM Pekka Paalanen > > > > > > > > > >>> wrote: > > > > > > > > > > > > > > > > > > On Wed, 22 Feb 2023 07:37:26 -0800 > > > > > > > > > Rob Clark wrote: > > > > > > > > > > > > > > > > > > > On Wed, Feb 22, 2023 at 1:49 AM Pekka Paalanen > > > > > > > > > > wrote: > > > > > > > > > >> > > > > > > > > > >> ... > > > > > > > > > >> > > > > > > > > > >> On another matter, if the application uses > > > > > > > > > >> SET_DEADLINE with one > > > > > > > > > >> timestamp, and the compositor uses SET_DEADLINE on > > > > > > > > > >> the same thing with > > > > > > > > > >> another timestamp, what should happen? > > > > > > > > > > > > > > > > > > > > The expectation is that many deadline hints can be > > > > > > > > > > set on a fence. > > > > > > > > > > The fence signaller should track the soonest > > > > > > > > > > deadline. > > > > > > > > > > > > > > > > > > You need to document that as UAPI, since it is > > > > > > > > > observable to userspace. > > > > > > > > > It would be bad if drivers or subsystems would > > > > > > > > > differ in behaviour. > > > > > > > > > > > > > > > > > > >>> > > > > > > > > > >>> It is in the end a hint. It is about giving the > > > > > > > > > >>> driver more > > > > > > > > > >>> information so that it can make better choices. But > > > > > > > > > >>> the driver is > > > > > > > > > >>> even free to ignore it. So maybe "expectation" is > > > > > > > > > >>> too strong of a > > > > > > > > > >>> word. Rather, any other behavior doesn't really make > > > > > > > > > >>> sense. But it > > > > > > > > > >>> could end up being dictated by how the hw and/or fw > > > > > > > > > >>> works. > > > > > > > > > >> > > > > > > > > > >> It will stop being a hint once it has been implemented > > > > > > > > > >> and used in the > > > > > > > > > >> wild long enough. The kernel userspace regression > > > > > > > > > >> rules make sure of > > > > > > > > > >> that. > > > > > > > > > > > > > > > > > > > > Yeah, tricky and maybe a gray area in this case. I > > > > > > > > > > think we eluded > > > > > > > > > > elsewhere in the thread that renaming the thing might > > > > > > > > > > be an option. > > > > > > > > > > > > > > > > > > > > So maybe instead of deadline, which is a very strong > > > > > > > > > > word, use something > > > > > > > > > > along the lines of "present time hint", or "signalled > > > > > > > > > > time hint"? Maybe > > > > > > > > > > reads clumsy. Just throwing some ideas for a start. > > > > > > > > > > > > > > > > > > You can try, but I fear that if it ever changes > > > > > > > > > behaviour and > > > > > > > > > someone notices that, it's labelled as a kernel > > > > > > > > > regression. I don't > > > > > > > > > think documentation has ever been the authoritative > > > > > > > > > definition of UABI > > > > > > > > > in Linux, it just guides drivers and userspace towards a > > > > > > > > > common > > > > > > > > > understanding and common usage patterns. > > > > > > > > > > > > > > > > > > So even if the UABI contract is not documented (ugh), > > > > > > > > > you need to be > > > > > > > > > prepared to set the UABI contract through kernel > > > > > > > > > implementation. > > > > > >
Re: [Freedreno] [PATCH v4 06/14] dma-buf/sync_file: Support (E)POLLPRI
On Mon, Feb 27, 2023 at 02:20:04PM -0800, Rob Clark wrote: > On Mon, Feb 27, 2023 at 1:36 PM Rodrigo Vivi wrote: > > > > On Fri, Feb 24, 2023 at 09:59:57AM -0800, Rob Clark wrote: > > > On Fri, Feb 24, 2023 at 7:27 AM Luben Tuikov wrote: > > > > > > > > On 2023-02-24 06:37, Tvrtko Ursulin wrote: > > > > > > > > > > On 24/02/2023 11:00, Pekka Paalanen wrote: > > > > >> On Fri, 24 Feb 2023 10:50:51 + > > > > >> Tvrtko Ursulin wrote: > > > > >> > > > > >>> On 24/02/2023 10:24, Pekka Paalanen wrote: > > > > On Fri, 24 Feb 2023 09:41:46 + > > > > Tvrtko Ursulin wrote: > > > > > > > > > On 24/02/2023 09:26, Pekka Paalanen wrote: > > > > >> On Thu, 23 Feb 2023 10:51:48 -0800 > > > > >> Rob Clark wrote: > > > > >> > > > > >>> On Thu, Feb 23, 2023 at 1:38 AM Pekka Paalanen > > > > >>> wrote: > > > > > > > > On Wed, 22 Feb 2023 07:37:26 -0800 > > > > Rob Clark wrote: > > > > > > > > > On Wed, Feb 22, 2023 at 1:49 AM Pekka Paalanen > > > > > wrote: > > > > >> > > > > >> ... > > > > >> > > > > >> On another matter, if the application uses SET_DEADLINE with > > > > >> one > > > > >> timestamp, and the compositor uses SET_DEADLINE on the same > > > > >> thing with > > > > >> another timestamp, what should happen? > > > > > > > > > > The expectation is that many deadline hints can be set on a > > > > > fence. > > > > > The fence signaller should track the soonest deadline. > > > > > > > > You need to document that as UAPI, since it is observable to > > > > userspace. > > > > It would be bad if drivers or subsystems would differ in > > > > behaviour. > > > > > > > > >>> > > > > >>> It is in the end a hint. It is about giving the driver more > > > > >>> information so that it can make better choices. But the driver > > > > >>> is > > > > >>> even free to ignore it. So maybe "expectation" is too strong > > > > >>> of a > > > > >>> word. Rather, any other behavior doesn't really make sense. > > > > >>> But it > > > > >>> could end up being dictated by how the hw and/or fw works. > > > > >> > > > > >> It will stop being a hint once it has been implemented and used > > > > >> in the > > > > >> wild long enough. The kernel userspace regression rules make > > > > >> sure of > > > > >> that. > > > > > > > > > > Yeah, tricky and maybe a gray area in this case. I think we eluded > > > > > elsewhere in the thread that renaming the thing might be an > > > > > option. > > > > > > > > > > So maybe instead of deadline, which is a very strong word, use > > > > > something > > > > > along the lines of "present time hint", or "signalled time hint"? > > > > > Maybe > > > > > reads clumsy. Just throwing some ideas for a start. > > > > > > > > You can try, but I fear that if it ever changes behaviour and > > > > someone notices that, it's labelled as a kernel regression. I don't > > > > think documentation has ever been the authoritative definition of > > > > UABI > > > > in Linux, it just guides drivers and userspace towards a common > > > > understanding and common usage patterns. > > > > > > > > So even if the UABI contract is not documented (ugh), you need to > > > > be > > > > prepared to set the UABI contract through kernel implementation. > > > > >>> > > > > >>> To be the devil's advocate it probably wouldn't be an ABI > > > > >>> regression but > > > > >>> just an regression. Same way as what nice(2) priorities mean hasn't > > > > >>> always been the same over the years, I don't think there is a strict > > > > >>> contract. > > > > >>> > > > > >>> Having said that, it may be different with latency sensitive stuff > > > > >>> such > > > > >>> as UIs though since it is very observable and can be very painful > > > > >>> to users. > > > > >>> > > > > If you do not document the UABI contract, then different drivers > > > > are > > > > likely to implement it differently, leading to differing behaviour. > > > > Also userspace will invent wild ways to abuse the UABI if there is > > > > no > > > > documentation guiding it on proper use. If userspace or end users > > > > observe different behaviour, that's bad even if it's not a > > > > regression. > > > > > > > > I don't like the situation either, but it is what it is. UABI > > > > stability > > > > trumps everything regardless of whether it was documented or not. > > > > > > > > I bet userspace is going to use this as a "make it faster, make it > > > > hotter" button. I would not be surprised if someone wrote a > > > > LD_PRELOAD > > > > library that stamps any and all fences with an
Re: [Freedreno] [PATCH v4 06/14] dma-buf/sync_file: Support (E)POLLPRI
On Tue, Feb 28, 2023 at 11:52 PM Rob Clark wrote: > > On Tue, Feb 28, 2023 at 6:30 AM Sebastian Wick > wrote: > > > > On Tue, Feb 28, 2023 at 12:48 AM Rob Clark wrote: > > > > > > On Mon, Feb 27, 2023 at 2:44 PM Sebastian Wick > > > wrote: > > > > > > > > On Mon, Feb 27, 2023 at 11:20 PM Rob Clark wrote: > > > > > > > > > > On Mon, Feb 27, 2023 at 1:36 PM Rodrigo Vivi > > > > > wrote: > > > > > > > > > > > > On Fri, Feb 24, 2023 at 09:59:57AM -0800, Rob Clark wrote: > > > > > > > On Fri, Feb 24, 2023 at 7:27 AM Luben Tuikov > > > > > > > wrote: > > > > > > > > > > > > > > > > On 2023-02-24 06:37, Tvrtko Ursulin wrote: > > > > > > > > > > > > > > > > > > On 24/02/2023 11:00, Pekka Paalanen wrote: > > > > > > > > >> On Fri, 24 Feb 2023 10:50:51 + > > > > > > > > >> Tvrtko Ursulin wrote: > > > > > > > > >> > > > > > > > > >>> On 24/02/2023 10:24, Pekka Paalanen wrote: > > > > > > > > On Fri, 24 Feb 2023 09:41:46 + > > > > > > > > Tvrtko Ursulin wrote: > > > > > > > > > > > > > > > > > On 24/02/2023 09:26, Pekka Paalanen wrote: > > > > > > > > >> On Thu, 23 Feb 2023 10:51:48 -0800 > > > > > > > > >> Rob Clark wrote: > > > > > > > > >> > > > > > > > > >>> On Thu, Feb 23, 2023 at 1:38 AM Pekka Paalanen > > > > > > > > >>> wrote: > > > > > > > > > > > > > > > > On Wed, 22 Feb 2023 07:37:26 -0800 > > > > > > > > Rob Clark wrote: > > > > > > > > > > > > > > > > > On Wed, Feb 22, 2023 at 1:49 AM Pekka Paalanen > > > > > > > > > wrote: > > > > > > > > >> > > > > > > > > >> ... > > > > > > > > >> > > > > > > > > >> On another matter, if the application uses > > > > > > > > >> SET_DEADLINE with one > > > > > > > > >> timestamp, and the compositor uses SET_DEADLINE on > > > > > > > > >> the same thing with > > > > > > > > >> another timestamp, what should happen? > > > > > > > > > > > > > > > > > > The expectation is that many deadline hints can be > > > > > > > > > set on a fence. > > > > > > > > > The fence signaller should track the soonest deadline. > > > > > > > > > > > > > > > > You need to document that as UAPI, since it is > > > > > > > > observable to userspace. > > > > > > > > It would be bad if drivers or subsystems would differ > > > > > > > > in behaviour. > > > > > > > > > > > > > > > > >>> > > > > > > > > >>> It is in the end a hint. It is about giving the driver > > > > > > > > >>> more > > > > > > > > >>> information so that it can make better choices. But > > > > > > > > >>> the driver is > > > > > > > > >>> even free to ignore it. So maybe "expectation" is too > > > > > > > > >>> strong of a > > > > > > > > >>> word. Rather, any other behavior doesn't really make > > > > > > > > >>> sense. But it > > > > > > > > >>> could end up being dictated by how the hw and/or fw > > > > > > > > >>> works. > > > > > > > > >> > > > > > > > > >> It will stop being a hint once it has been implemented > > > > > > > > >> and used in the > > > > > > > > >> wild long enough. The kernel userspace regression rules > > > > > > > > >> make sure of > > > > > > > > >> that. > > > > > > > > > > > > > > > > > > Yeah, tricky and maybe a gray area in this case. I think > > > > > > > > > we eluded > > > > > > > > > elsewhere in the thread that renaming the thing might be > > > > > > > > > an option. > > > > > > > > > > > > > > > > > > So maybe instead of deadline, which is a very strong > > > > > > > > > word, use something > > > > > > > > > along the lines of "present time hint", or "signalled > > > > > > > > > time hint"? Maybe > > > > > > > > > reads clumsy. Just throwing some ideas for a start. > > > > > > > > > > > > > > > > You can try, but I fear that if it ever changes behaviour > > > > > > > > and > > > > > > > > someone notices that, it's labelled as a kernel > > > > > > > > regression. I don't > > > > > > > > think documentation has ever been the authoritative > > > > > > > > definition of UABI > > > > > > > > in Linux, it just guides drivers and userspace towards a > > > > > > > > common > > > > > > > > understanding and common usage patterns. > > > > > > > > > > > > > > > > So even if the UABI contract is not documented (ugh), you > > > > > > > > need to be > > > > > > > > prepared to set the UABI contract through kernel > > > > > > > > implementation. > > > > > > > > >>> > > > > > > > > >>> To be the devil's advocate it probably wouldn't be an ABI > > > > > > > > >>> regression but > > > > > > > > >>> just an regression. Same way as what nice(2) priorities > > > > > > > > >>> mean hasn't > > > > > > > > >>> always been the same over the years, I don't
Re: [Freedreno] [PATCH AUTOSEL 6.2 18/60] drm/msm/dp: Remove INIT_SETUP delay
On Mon, Feb 27, 2023 at 10:12:20AM +0100, Johan Hovold wrote: On Sun, Feb 26, 2023 at 09:00:03PM -0500, Sasha Levin wrote: From: Bjorn Andersson [ Upstream commit e17af1c9d861dc177e5b56009bd4f71ace688d97 ] During initalization of the DisplayPort controller an EV_HPD_INIT_SETUP event is generated, but with a delay of 100 units. This delay existed to circumvent bug in the QMP combo PHY driver, where if the DP part was powered up before USB, the common properties would not be properly initialized - and USB wouldn't work. This issue was resolved in the recent refactoring of the QMP driver, so it's now possible to remove this delay. While there is still a timing dependency in the current implementation, test indicates that it's now possible to boot with an external display on USB Type-C and have the display power up, without disconnecting and reconnecting the cable. Signed-off-by: Bjorn Andersson Reviewed-by: Kuogee Hsieh Patchwork: https://patchwork.freedesktop.org/patch/518729/ Link: https://lore.kernel.org/r/20230117172951.2748456-1-quic_bjora...@quicinc.com Signed-off-by: Dmitry Baryshkov Signed-off-by: Sasha Levin This is not a bug fix and should not be backported. Ack, I'll drop all the INIT_SETUP patches. Thanks! -- Thanks, Sasha
Re: [Freedreno] [PATCH] drm/msm: Initialize mode_config earlier
On Tue, Jan 24, 2023 at 09:09:02AM +0100, Johan Hovold wrote: > On Mon, Jan 23, 2023 at 09:17:49AM -0800, Bjorn Andersson wrote: > > On Mon, Jan 23, 2023 at 05:01:45PM +0100, Johan Hovold wrote: > > > On Tue, Jan 17, 2023 at 09:04:39AM +0100, Johan Hovold wrote: > > > > On Mon, Jan 16, 2023 at 08:51:22PM -0600, Bjorn Andersson wrote: > > > > > > Perhaps we have shuffled other things around to avoid this bug? > > > > > Either > > > > > way, let's this on hold until further proof that it's still > > > > > reproducible. > > > > > > > > As I've mentioned off list, I haven't hit the apparent race I reported > > > > here: > > > > > > > > > > > > https://lore.kernel.org/all/y1efjh11b5uqz...@hovoldconsulting.com/ > > > > > > > > since moving to 6.2. I did hit it with both 6.0 and 6.1-rc2, but it > > > > could very well be that something has changes that fixes (or hides) the > > > > issue since. > > > > > > For unrelated reasons, I tried enabling async probing, and apart from > > > apparently causing the panel driver to probe defer indefinitely, I also > > > again hit the WARN_ON() I had added to catch this: > > > > > > [ 13.593235] WARNING: CPU: 0 PID: 125 at > > > drivers/gpu/drm/drm_probe_helper.c:664 > > > drm_kms_helper_hotplug_event+0x48/0x7 > > > 0 [drm_kms_helper] > > > > So the bug still appears to be there (and the MSM DRM driver is fragile > > > and broken, but we knew that). > > > > > > > But the ordering between mode_config.funcs = !NULL and > > drm_kms_helper_poll_init() in msm_drm_init() seems pretty clear. > > > > And my testing shows that drm_kms_helper_poll_init() is the cause for > > getting bridge->hpd_cb != NULL. > > > > So the ordering seems legit, unless there's something else causing the > > assignment of bridge->hpd_cb to happen earlier in this scenario. > > I'm not saying that this patch is correct (indeed it doesn't seem to > be), but only that the bug I reported still appears to be present in > 6.2. So after debugging this issue a third time, I can conclude that it is still very much present in 6.2. It appears you looked at the linux-next tree when you concluded that this patch was not needed. In 6.2 the bridge->hpd_cb callback is set before mode_config.funcs is initialised as part of kms->funcs->hw_init(kms). The hpd DRM changes heading into 6.3 do appear to avoid the NULL-pointer dereference by moving the bridge->hpd_cb initialisation to drm_kms_helper_poll_init() as you mention above. The PMIC GLINK altmode driver still happily forwards notifications regardless of the DRM driver state though, which can lead to missed hotplug events. It seems you need to implement the hpd_enable()/disable() callbacks and either cache or not enable events in fw until the DRM driver is ready. Johan
Re: [Freedreno] [RFC PATCH 1/2] drm/msm/dp: enumerate edp panel during driver probe
On 01/03/2023 10:13, Sankeerth Billakanti (QUIC) wrote:
The eDP panel is identified and enumerated during probe of the
panel-edp driver. The current DP driver triggers this panel-edp driver
probe while getting the panel-bridge associated with the eDP panel
from the platform driver bind. If the panel-edp probe is deferred,
then the whole bunch of MDSS parent and child drivers have to defer and
roll back.
No, MDSS has not been rolled back since 5.19. Please shift your development
on top of the current msm-next.
Okay, I will move to the msm-next tip.
So, we want to move the panel enumeration from bind to probe so that
the probe defer is easier to handle and also both the drivers appear
consistent in panel enumeration. This change moves the DP driver
panel-bridge enumeration to probe.
Signed-off-by: Sankeerth Billakanti
---
drivers/gpu/drm/msm/dp/dp_aux.c | 149
++--
drivers/gpu/drm/msm/dp/dp_catalog.c | 12 +++
drivers/gpu/drm/msm/dp/dp_catalog.h | 1 +
drivers/gpu/drm/msm/dp/dp_display.c | 80 ++-
4 files changed, 182 insertions(+), 60 deletions(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c
b/drivers/gpu/drm/msm/dp/dp_aux.c index cc3efed593aa..5da95dfdeede
100644
--- a/drivers/gpu/drm/msm/dp/dp_aux.c
+++ b/drivers/gpu/drm/msm/dp/dp_aux.c
@@ -110,7 +110,7 @@ static ssize_t dp_aux_write(struct dp_aux_private
*aux, }
static ssize_t dp_aux_cmd_fifo_tx(struct dp_aux_private *aux,
- struct drm_dp_aux_msg *msg)
+ struct drm_dp_aux_msg *msg, bool poll)
What is the reason for working in polled mode rather than always using the
interrupts?
The mdss interrupts will be enabled and can be used after msm_irq_install from
msm_drm_bind.
We want to perform aux transactions during probe. The aux data transmission is
followed by an
interrupt to indicate successful transmission status and reply information.
This is the price of developing on, let me guess, 5.15. Nowadays MDSS
interrupts are enabled and can be used during dp_display_probe() and
dp_display_bind(). If they can not for some reason, this is an issue
that must be fixed. Please reconsider your approach after rebasing onto
msm-next or 6.3-rc1.
As interrupts are not enabled, I took this polling approach for aux interrupts
during probe.
{
ssize_t ret;
unsigned long time_left;
@@ -121,10 +121,16 @@ static ssize_t dp_aux_cmd_fifo_tx(struct
dp_aux_private *aux,
if (ret < 0)
return ret;
- time_left = wait_for_completion_timeout(>comp,
+ if (!poll) {
+ time_left = wait_for_completion_timeout(>comp,
msecs_to_jiffies(250));
- if (!time_left)
- return -ETIMEDOUT;
+ if (!time_left)
+ return -ETIMEDOUT;
+ } else {
+ ret = dp_catalog_aux_poll_and_get_hw_interrupt(aux->catalog);
+ if (!ret)
+ dp_aux_isr(>dp_aux);
+ }
return ret;
}
@@ -238,7 +244,7 @@ static void
dp_aux_update_offset_and_segment(struct dp_aux_private *aux,
*/
static void dp_aux_transfer_helper(struct dp_aux_private *aux,
struct drm_dp_aux_msg *input_msg,
- bool send_seg)
+ bool send_seg, bool poll)
{
struct drm_dp_aux_msg helper_msg;
u32 message_size = 0x10;
@@ -278,7 +284,7 @@ static void dp_aux_transfer_helper(struct
dp_aux_private *aux,
helper_msg.address = segment_address;
helper_msg.buffer = >segment;
helper_msg.size = 1;
- dp_aux_cmd_fifo_tx(aux, _msg);
+ dp_aux_cmd_fifo_tx(aux, _msg, poll);
}
/*
@@ -292,7 +298,7 @@ static void dp_aux_transfer_helper(struct
dp_aux_private *aux,
helper_msg.address = input_msg->address;
helper_msg.buffer = >offset;
helper_msg.size = 1;
- dp_aux_cmd_fifo_tx(aux, _msg);
+ dp_aux_cmd_fifo_tx(aux, _msg, poll);
end:
aux->offset += message_size;
@@ -300,6 +306,122 @@ static void dp_aux_transfer_helper(struct
dp_aux_private *aux,
aux->segment = 0x0; /* reset segment at end of block
*/ }
+/*
+ * This function does the real job to process an AUX transaction.
+ * It will call aux_reset() function to reset the AUX channel,
+ * if the waiting is timeout.
+ */
+static ssize_t dp_aux_transfer_init(struct drm_dp_aux *dp_aux,
+ struct drm_dp_aux_msg *msg) {
+ ssize_t ret;
+ int const aux_cmd_native_max = 16;
+ int const aux_cmd_i2c_max = 128;
+ struct dp_aux_private *aux;
+
+ aux = container_of(dp_aux, struct dp_aux_private, dp_aux);
+
+ aux->native = msg->request & (DP_AUX_NATIVE_WRITE &
+ DP_AUX_NATIVE_READ);
+
+ /* Ignore address
Re: [Freedreno] [PATCH v4 3/4] drm/msm/dpu: Remove empty prepare_commit() function
On 2023-03-01 11:08:16, Marijn Suijten wrote:
> On 2023-02-21 10:42:55, Jessica Zhang wrote:
> > Now that the TE setup has been moved to prepare_for_kickoff(), we have
> > not prepare_commit() callbacks left. This makes dpu_encoder_prepare_commit()
>
> s/not/no
>
> > do nothing. Remove prepare_commit() from DPU driver.
>
> And again, this:
>
> > Changes in V3:
> > - Reworded commit message to be more clear
> > - Corrected spelling mistake in commit message
> >
> > Changes in V4:
> > - Reworded commit message for clarity
>
> ... should go below the cut.
>
> > Signed-off-by: Jessica Zhang
>
> With the above two issues fixed:
>
> Reviewed-by: Marijn Suijten
>
> > ---
> > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 19 ---
> > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 7 ---
> > drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 21 -
> > 3 files changed, 47 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > index dcceed91aed8..35e120b5ef53 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > @@ -2090,25 +2090,6 @@ void dpu_encoder_helper_phys_cleanup(struct
> > dpu_encoder_phys *phys_enc)
> > ctl->ops.clear_pending_flush(ctl);
> > }
> >
> > -void dpu_encoder_prepare_commit(struct drm_encoder *drm_enc)
> > -{
> > - struct dpu_encoder_virt *dpu_enc;
> > - struct dpu_encoder_phys *phys;
> > - int i;
> > -
> > - if (!drm_enc) {
> > - DPU_ERROR("invalid encoder\n");
> > - return;
> > - }
> > - dpu_enc = to_dpu_encoder_virt(drm_enc);
> > -
> > - for (i = 0; i < dpu_enc->num_phys_encs; i++) {
> > - phys = dpu_enc->phys_encs[i];
> > - if (phys->ops.prepare_commit)
> > - phys->ops.prepare_commit(phys);
In hindsight, Dmitry asked in v2 to remove prepare_commit from
dpu_encoder_phys_ops (and its documentation comment) in
dpu_encoder_phys.h, but that has not happened yet. Can we do that in a
v5?
- Marijn
> > - }
> > -}
> > -
> > #ifdef CONFIG_DEBUG_FS
> > static int _dpu_encoder_status_show(struct seq_file *s, void *data)
> > {
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> > b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> > index 9e7236ef34e6..2c9ef8d1b877 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> > @@ -146,13 +146,6 @@ struct drm_encoder *dpu_encoder_init(
> > int dpu_encoder_setup(struct drm_device *dev, struct drm_encoder *enc,
> > struct msm_display_info *disp_info);
> >
> > -/**
> > - * dpu_encoder_prepare_commit - prepare encoder at the very beginning of an
> > - * atomic commit, before any registers are written
> > - * @drm_enc:Pointer to previously created drm encoder structure
> > - */
> > -void dpu_encoder_prepare_commit(struct drm_encoder *drm_enc);
> > -
> > /**
> > * dpu_encoder_set_idle_timeout - set the idle timeout for video
> > *and command mode encoders.
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> > b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> > index 165958d47ec6..6f7ddbf0d9b7 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> > @@ -425,26 +425,6 @@ static ktime_t dpu_kms_vsync_time(struct msm_kms *kms,
> > struct drm_crtc *crtc)
> > return ktime_get();
> > }
> >
> > -static void dpu_kms_prepare_commit(struct msm_kms *kms,
> > - struct drm_atomic_state *state)
> > -{
> > - struct drm_crtc *crtc;
> > - struct drm_crtc_state *crtc_state;
> > - struct drm_encoder *encoder;
> > - int i;
> > -
> > - if (!kms)
> > - return;
> > -
> > - /* Call prepare_commit for all affected encoders */
> > - for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
> > - drm_for_each_encoder_mask(encoder, crtc->dev,
> > - crtc_state->encoder_mask) {
> > - dpu_encoder_prepare_commit(encoder);
> > - }
> > - }
> > -}
> > -
> > static void dpu_kms_flush_commit(struct msm_kms *kms, unsigned crtc_mask)
> > {
> > struct dpu_kms *dpu_kms = to_dpu_kms(kms);
> > @@ -949,7 +929,6 @@ static const struct msm_kms_funcs kms_funcs = {
> > .enable_commit = dpu_kms_enable_commit,
> > .disable_commit = dpu_kms_disable_commit,
> > .vsync_time = dpu_kms_vsync_time,
> > - .prepare_commit = dpu_kms_prepare_commit,
> > .flush_commit= dpu_kms_flush_commit,
> > .wait_flush = dpu_kms_wait_flush,
> > .complete_commit = dpu_kms_complete_commit,
> > --
> > 2.39.2
> >
Re: [Freedreno] [PATCH v4 3/4] drm/msm/dpu: Remove empty prepare_commit() function
On 2023-02-21 10:42:55, Jessica Zhang wrote:
> Now that the TE setup has been moved to prepare_for_kickoff(), we have
> not prepare_commit() callbacks left. This makes dpu_encoder_prepare_commit()
s/not/no
> do nothing. Remove prepare_commit() from DPU driver.
And again, this:
> Changes in V3:
> - Reworded commit message to be more clear
> - Corrected spelling mistake in commit message
>
> Changes in V4:
> - Reworded commit message for clarity
... should go below the cut.
> Signed-off-by: Jessica Zhang
With the above two issues fixed:
Reviewed-by: Marijn Suijten
> ---
> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 19 ---
> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 7 ---
> drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 21 -
> 3 files changed, 47 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> index dcceed91aed8..35e120b5ef53 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -2090,25 +2090,6 @@ void dpu_encoder_helper_phys_cleanup(struct
> dpu_encoder_phys *phys_enc)
> ctl->ops.clear_pending_flush(ctl);
> }
>
> -void dpu_encoder_prepare_commit(struct drm_encoder *drm_enc)
> -{
> - struct dpu_encoder_virt *dpu_enc;
> - struct dpu_encoder_phys *phys;
> - int i;
> -
> - if (!drm_enc) {
> - DPU_ERROR("invalid encoder\n");
> - return;
> - }
> - dpu_enc = to_dpu_encoder_virt(drm_enc);
> -
> - for (i = 0; i < dpu_enc->num_phys_encs; i++) {
> - phys = dpu_enc->phys_encs[i];
> - if (phys->ops.prepare_commit)
> - phys->ops.prepare_commit(phys);
> - }
> -}
> -
> #ifdef CONFIG_DEBUG_FS
> static int _dpu_encoder_status_show(struct seq_file *s, void *data)
> {
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> index 9e7236ef34e6..2c9ef8d1b877 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> @@ -146,13 +146,6 @@ struct drm_encoder *dpu_encoder_init(
> int dpu_encoder_setup(struct drm_device *dev, struct drm_encoder *enc,
> struct msm_display_info *disp_info);
>
> -/**
> - * dpu_encoder_prepare_commit - prepare encoder at the very beginning of an
> - * atomic commit, before any registers are written
> - * @drm_enc:Pointer to previously created drm encoder structure
> - */
> -void dpu_encoder_prepare_commit(struct drm_encoder *drm_enc);
> -
> /**
> * dpu_encoder_set_idle_timeout - set the idle timeout for video
> *and command mode encoders.
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> index 165958d47ec6..6f7ddbf0d9b7 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> @@ -425,26 +425,6 @@ static ktime_t dpu_kms_vsync_time(struct msm_kms *kms,
> struct drm_crtc *crtc)
> return ktime_get();
> }
>
> -static void dpu_kms_prepare_commit(struct msm_kms *kms,
> - struct drm_atomic_state *state)
> -{
> - struct drm_crtc *crtc;
> - struct drm_crtc_state *crtc_state;
> - struct drm_encoder *encoder;
> - int i;
> -
> - if (!kms)
> - return;
> -
> - /* Call prepare_commit for all affected encoders */
> - for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
> - drm_for_each_encoder_mask(encoder, crtc->dev,
> - crtc_state->encoder_mask) {
> - dpu_encoder_prepare_commit(encoder);
> - }
> - }
> -}
> -
> static void dpu_kms_flush_commit(struct msm_kms *kms, unsigned crtc_mask)
> {
> struct dpu_kms *dpu_kms = to_dpu_kms(kms);
> @@ -949,7 +929,6 @@ static const struct msm_kms_funcs kms_funcs = {
> .enable_commit = dpu_kms_enable_commit,
> .disable_commit = dpu_kms_disable_commit,
> .vsync_time = dpu_kms_vsync_time,
> - .prepare_commit = dpu_kms_prepare_commit,
> .flush_commit= dpu_kms_flush_commit,
> .wait_flush = dpu_kms_wait_flush,
> .complete_commit = dpu_kms_complete_commit,
> --
> 2.39.2
>
Re: [Freedreno] [PATCH v4 4/4] drm/msm/mdp4: Remove empty prepare_commit() function
On 2023-02-21 10:42:56, Jessica Zhang wrote:
> Remove empty prepare_commit() function from MDP4 driver.
>
> Signed-off-by: Jessica Zhang
> Reviewed-by: Dmitry Baryshkov
Reviewed-by: Marijn Suijten
> ---
> drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c | 5 -
> 1 file changed, 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
> b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
> index 9a1a0769575d..6e37072ed302 100644
> --- a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
> +++ b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
> @@ -84,10 +84,6 @@ static void mdp4_disable_commit(struct msm_kms *kms)
> mdp4_disable(mdp4_kms);
> }
>
> -static void mdp4_prepare_commit(struct msm_kms *kms, struct drm_atomic_state
> *state)
> -{
> -}
> -
> static void mdp4_flush_commit(struct msm_kms *kms, unsigned crtc_mask)
> {
> /* TODO */
> @@ -154,7 +150,6 @@ static const struct mdp_kms_funcs kms_funcs = {
> .disable_vblank = mdp4_disable_vblank,
> .enable_commit = mdp4_enable_commit,
> .disable_commit = mdp4_disable_commit,
> - .prepare_commit = mdp4_prepare_commit,
> .flush_commit= mdp4_flush_commit,
> .wait_flush = mdp4_wait_flush,
> .complete_commit = mdp4_complete_commit,
> --
> 2.39.2
>
Re: [Freedreno] [PATCH v4 2/4] drm/msm: Check for NULL before calling prepare_commit()
On 2023-02-21 10:42:54, Jessica Zhang wrote: > Add a NULL check before calling prepare_commit() in > msm_atomic_commit_tail() > > Signed-off-by: Jessica Zhang > Reviewed-by: Dmitry Baryshkov Reviewed-by: Marijn Suijten > --- > drivers/gpu/drm/msm/msm_atomic.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/msm/msm_atomic.c > b/drivers/gpu/drm/msm/msm_atomic.c > index 1686fbb611fd..c8a0a5cc5ca5 100644 > --- a/drivers/gpu/drm/msm/msm_atomic.c > +++ b/drivers/gpu/drm/msm/msm_atomic.c > @@ -206,7 +206,8 @@ void msm_atomic_commit_tail(struct drm_atomic_state > *state) >* Now that there is no in-progress flush, prepare the >* current update: >*/ > - kms->funcs->prepare_commit(kms, state); > + if (kms->funcs->prepare_commit) > + kms->funcs->prepare_commit(kms, state); > > /* >* Push atomic updates down to hardware: > -- > 2.39.2 >
Re: [Freedreno] [PATCH v4 1/4] drm/msm/dpu: Move TE setup to prepare_for_kickoff()
On 2023-02-21 10:42:53, Jessica Zhang wrote:
> Currently, DPU will enable TE during prepare_commit(). However, this
> will cause a crash and reboot to sahara when trying to read/write to
> register in get_autorefresh_config(), because the core clock rates
> aren't set at that time.
Haven't seeen a crash like this on any of my devices (after implementing
INTF TE). get_autorefresh_config() always reads zero (or 1 for
frame_count) except the first time it is called (autorefresh is left
enabled by our bootloader on SM6125) and triggers the disable codepath.
It does look like prepare_for_kickoff() is much more susceptible to
delays in code than prepare_commit(). The debug logs used to figure
this out together with this series result in FPS drops on SM6125 and
SM8150. Not an issue with this series, just something to keep in mind.
> This used to work because phys_enc->hw_pp is only initialized in mode
> set [1], so the first prepare_commit() will return before any register
> read/write as hw_pp would be NULL.
>
> However, when we try to implement support for INTF TE, we will run into
> the clock issue described above as hw_intf will *not* be NULL on the
> first prepare_commit(). This is because the initialization of
> dpu_enc->hw_intf has been moved to dpu_encoder_setup() [2].
>
> To avoid this issue, let's enable TE during prepare_for_kickoff()
> instead as the core clock rates are guaranteed to be set then.
For the change itself:
Reviewed-by: Marijn Suijten
And tested on SM6125, SM8150 (INTF TE) and SDM845 (PP TE).
Then, for some patch hygiene, starting here:
> Depends on: "Implement tearcheck support on INTF block" [3]
>
> Changes in V3:
> - Added function prototypes
> - Reordered function definitions to make change more legible
> - Removed prepare_commit() function from dpu_encoder_phys_cmd
>
> Changes in V4:
> - Reworded commit message to be more specific
> - Removed dpu_encoder_phys_cmd_is_ongoing_pptx() prototype
... until here: all this info belongs /below the cut/ outside of the
messge that becomes part of the commit when this patch is applied to the
tree.
> [1]
> https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c#L1109
> [2]
> https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c#L2339
Please replace these with "permalinks" (to a commit hash): a branch with
line number annotation will fall out of date soon as more patches are
applied that touch these files.
> [3] https://patchwork.freedesktop.org/series/112332/
Is this a hard dependency? It seems this series applies cleanly on
-next and - from a cursory view - should be applicable and testable
without my INTF TE series. However, Dmitry asked me to move some code
around in review resulting in separate callbacks in the encoder, rather
than having various if(has_intf_te) within those callbacks. That'll
cause conflicts when I eventually get to respin a v2.
- Marijn
> Signed-off-by: Jessica Zhang
> ---
> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 8 +---
> 1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
> index cb05036f2916..98958c75864a 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
> @@ -40,6 +40,8 @@
>
> #define DPU_ENC_MAX_POLL_TIMEOUT_US 2000
>
> +static void dpu_encoder_phys_cmd_enable_te(struct dpu_encoder_phys
> *phys_enc);
> +
> static bool dpu_encoder_phys_cmd_is_master(struct dpu_encoder_phys *phys_enc)
> {
> return (phys_enc->split_role != ENC_ROLE_SLAVE);
> @@ -611,6 +613,8 @@ static void dpu_encoder_phys_cmd_prepare_for_kickoff(
> phys_enc->hw_pp->idx - PINGPONG_0);
> }
>
> + dpu_encoder_phys_cmd_enable_te(phys_enc);
> +
> DPU_DEBUG_CMDENC(cmd_enc, "pp:%d pending_cnt %d\n",
> phys_enc->hw_pp->idx - PINGPONG_0,
> atomic_read(_enc->pending_kickoff_cnt));
> @@ -641,8 +645,7 @@ static bool dpu_encoder_phys_cmd_is_ongoing_pptx(
> return false;
> }
>
> -static void dpu_encoder_phys_cmd_prepare_commit(
> - struct dpu_encoder_phys *phys_enc)
> +static void dpu_encoder_phys_cmd_enable_te(struct dpu_encoder_phys *phys_enc)
> {
> struct dpu_encoder_phys_cmd *cmd_enc =
> to_dpu_encoder_phys_cmd(phys_enc);
> @@ -799,7 +802,6 @@ static void dpu_encoder_phys_cmd_trigger_start(
> static void dpu_encoder_phys_cmd_init_ops(
> struct dpu_encoder_phys_ops *ops)
> {
> - ops->prepare_commit = dpu_encoder_phys_cmd_prepare_commit;
> ops->is_master = dpu_encoder_phys_cmd_is_master;
> ops->atomic_mode_set = dpu_encoder_phys_cmd_atomic_mode_set;
> ops->enable = dpu_encoder_phys_cmd_enable;
> --
> 2.39.2
>
Re: [Freedreno] [PATCH v8 08/16] dma-buf/sw_sync: Add fence deadline support
On Tue, 28 Feb 2023 14:58:12 -0800
Rob Clark wrote:
> From: Rob Clark
>
> This consists of simply storing the most recent deadline, and adding an
> ioctl to retrieve the deadline. This can be used in conjunction with
> the SET_DEADLINE ioctl on a fence fd for testing. Ie. create various
> sw_sync fences, merge them into a fence-array, set deadline on the
> fence-array and confirm that it is propagated properly to each fence.
>
> v2: Switch UABI to express deadline as u64
> v3: More verbose UAPI docs, show how to convert from timespec
> v4: Better comments, track the soonest deadline, as a normal fence
> implementation would, return an error if no deadline set.
>
> Signed-off-by: Rob Clark
> Reviewed-by: Christian König
> ---
> drivers/dma-buf/sw_sync.c| 81
> drivers/dma-buf/sync_debug.h | 2 +
> 2 files changed, 83 insertions(+)
>
> diff --git a/drivers/dma-buf/sw_sync.c b/drivers/dma-buf/sw_sync.c
> index 348b3a9170fa..f53071bca3af 100644
> --- a/drivers/dma-buf/sw_sync.c
> +++ b/drivers/dma-buf/sw_sync.c
> @@ -52,12 +52,33 @@ struct sw_sync_create_fence_data {
> __s32 fence; /* fd of new fence */
> };
>
> +/**
> + * struct sw_sync_get_deadline - get the deadline hint of a sw_sync fence
> + * @deadline_ns: absolute time of the deadline
> + * @pad: must be zero
> + * @fence_fd:the sw_sync fence fd (in)
> + *
> + * Return the earliest deadline set on the fence. The timebase for the
> + * deadline is CLOCK_MONOTONIC (same as vblank). If there is no deadline
> + * set on the fence, this ioctl will return -ENOENT.
> + */
> +struct sw_sync_get_deadline {
> + __u64 deadline_ns;
> + __u32 pad;
> + __s32 fence_fd;
> +};
Sounds good.
> diff --git a/drivers/dma-buf/sync_debug.h b/drivers/dma-buf/sync_debug.h
> index 6176e52ba2d7..2e0146d0bdbb 100644
> --- a/drivers/dma-buf/sync_debug.h
> +++ b/drivers/dma-buf/sync_debug.h
> @@ -55,11 +55,13 @@ static inline struct sync_timeline
> *dma_fence_parent(struct dma_fence *fence)
> * @base: base fence object
> * @link: link on the sync timeline's list
> * @node: node in the sync timeline's tree
> + * @deadline: the most recently set fence deadline
Now it's the earliest deadline.
> */
> struct sync_pt {
> struct dma_fence base;
> struct list_head link;
> struct rb_node node;
> + ktime_t deadline;
> };
>
> extern const struct file_operations sw_sync_debugfs_fops;
Acked-by: Pekka Paalanen
Thanks,
pq
pgplxgMTKVCVN.pgp
Description: OpenPGP digital signature
Re: [Freedreno] [PATCH v8 05/16] dma-buf/sync_file: Surface sync-file uABI
On Tue, 28 Feb 2023 14:58:09 -0800
Rob Clark wrote:
> From: Rob Clark
>
> We had all of the internal driver APIs, but not the all important
> userspace uABI, in the dma-buf doc. Fix that. And re-arrange the
> comments slightly as otherwise the comments for the ioctl nr defines
> would not show up.
>
> Signed-off-by: Rob Clark
> ---
> Documentation/driver-api/dma-buf.rst | 10 ++--
> include/uapi/linux/sync_file.h | 35 +++-
> 2 files changed, 22 insertions(+), 23 deletions(-)
>
Sounds good.
Acked-by: Pekka Paalanen
Thanks,
pq
> diff --git a/Documentation/driver-api/dma-buf.rst
> b/Documentation/driver-api/dma-buf.rst
> index 183e480d8cea..ff3f8da296af 100644
> --- a/Documentation/driver-api/dma-buf.rst
> +++ b/Documentation/driver-api/dma-buf.rst
> @@ -203,8 +203,8 @@ DMA Fence unwrap
> .. kernel-doc:: include/linux/dma-fence-unwrap.h
> :internal:
>
> -DMA Fence uABI/Sync File
> -
> +DMA Fence Sync File
> +~~~
>
> .. kernel-doc:: drivers/dma-buf/sync_file.c
> :export:
> @@ -212,6 +212,12 @@ DMA Fence uABI/Sync File
> .. kernel-doc:: include/linux/sync_file.h
> :internal:
>
> +DMA Fence Sync File uABI
> +
> +
> +.. kernel-doc:: include/uapi/linux/sync_file.h
> + :internal:
> +
> Indefinite DMA Fences
> ~
>
> diff --git a/include/uapi/linux/sync_file.h b/include/uapi/linux/sync_file.h
> index ee2dcfb3d660..eced40c204d7 100644
> --- a/include/uapi/linux/sync_file.h
> +++ b/include/uapi/linux/sync_file.h
> @@ -16,12 +16,16 @@
> #include
>
> /**
> - * struct sync_merge_data - data passed to merge ioctl
> + * struct sync_merge_data - SYNC_IOC_MERGE: merge two fences
> * @name:name of new fence
> * @fd2: file descriptor of second fence
> * @fence: returns the fd of the new fence to userspace
> * @flags: merge_data flags
> * @pad: padding for 64-bit alignment, should always be zero
> + *
> + * Creates a new fence containing copies of the sync_pts in both
> + * the calling fd and sync_merge_data.fd2. Returns the new fence's
> + * fd in sync_merge_data.fence
> */
> struct sync_merge_data {
> charname[32];
> @@ -34,8 +38,8 @@ struct sync_merge_data {
> /**
> * struct sync_fence_info - detailed fence information
> * @obj_name:name of parent sync_timeline
> -* @driver_name: name of driver implementing the parent
> -* @status: status of the fence 0:active 1:signaled <0:error
> + * @driver_name: name of driver implementing the parent
> + * @status: status of the fence 0:active 1:signaled <0:error
> * @flags: fence_info flags
> * @timestamp_ns:timestamp of status change in nanoseconds
> */
> @@ -48,14 +52,19 @@ struct sync_fence_info {
> };
>
> /**
> - * struct sync_file_info - data returned from fence info ioctl
> + * struct sync_file_info - SYNC_IOC_FILE_INFO: get detailed information on a
> sync_file
> * @name:name of fence
> * @status: status of fence. 1: signaled 0:active <0:error
> * @flags: sync_file_info flags
> * @num_fences number of fences in the sync_file
> * @pad: padding for 64-bit alignment, should always be zero
> - * @sync_fence_info: pointer to array of structs sync_fence_info with all
> + * @sync_fence_info: pointer to array of struct _fence_info with all
> *fences in the sync_file
> + *
> + * Takes a struct sync_file_info. If num_fences is 0, the field is updated
> + * with the actual number of fences. If num_fences is > 0, the system will
> + * use the pointer provided on sync_fence_info to return up to num_fences of
> + * struct sync_fence_info, with detailed fence information.
> */
> struct sync_file_info {
> charname[32];
> @@ -76,23 +85,7 @@ struct sync_file_info {
> * no upstream users available.
> */
>
> -/**
> - * DOC: SYNC_IOC_MERGE - merge two fences
> - *
> - * Takes a struct sync_merge_data. Creates a new fence containing copies of
> - * the sync_pts in both the calling fd and sync_merge_data.fd2. Returns the
> - * new fence's fd in sync_merge_data.fence
> - */
> #define SYNC_IOC_MERGE _IOWR(SYNC_IOC_MAGIC, 3, struct
> sync_merge_data)
> -
> -/**
> - * DOC: SYNC_IOC_FILE_INFO - get detailed information on a sync_file
> - *
> - * Takes a struct sync_file_info. If num_fences is 0, the field is updated
> - * with the actual number of fences. If num_fences is > 0, the system will
> - * use the pointer provided on sync_fence_info to return up to num_fences of
> - * struct sync_fence_info, with detailed fence information.
> - */
> #define SYNC_IOC_FILE_INFO _IOWR(SYNC_IOC_MAGIC, 4, struct sync_file_info)
>
> #endif /* _UAPI_LINUX_SYNC_H */
pgpVPqtS7gNBS.pgp
Description: OpenPGP digital signature
Re: [Freedreno] [PATCH v8 01/16] dma-buf/dma-fence: Add deadline awareness
On 3/1/23 05:58, Rob Clark wrote: > From: Rob Clark > > Add a way to hint to the fence signaler of an upcoming deadline, such as > vblank, which the fence waiter would prefer not to miss. This is to aid > the fence signaler in making power management decisions, like boosting > frequency as the deadline approaches and awareness of missing deadlines > so that can be factored in to the frequency scaling. > > v2: Drop dma_fence::deadline and related logic to filter duplicate > deadlines, to avoid increasing dma_fence size. The fence-context > implementation will need similar logic to track deadlines of all > the fences on the same timeline. [ckoenig] > v3: Clarify locking wrt. set_deadline callback > v4: Clarify in docs comment that this is a hint > v5: Drop DMA_FENCE_FLAG_HAS_DEADLINE_BIT. > v6: More docs > v7: Fix typo, clarify past deadlines > > Signed-off-by: Rob Clark > Reviewed-by: Christian König > Acked-by: Pekka Paalanen I have given my Reviewed-by from v7 [1], but it didn't get picked up, thus giving it again: Reviewed-by: Bagas Sanjaya Thanks. [1]: https://lore.kernel.org/linux-doc/y%2f7lflxhijdpd...@debian.me/ -- An old man doll... just what I always wanted! - Clara
Re: [Freedreno] [PATCH v4 3/4] drm/msm/dpu: avoid unnecessary check in DPU reservations
On 2023-02-13 03:11:43, Kalyan Thota wrote:
> Return immediately on failure, this will make dpu reservations
> part look cleaner.
>
> Signed-off-by: Kalyan Thota
> Reviewed-by: Dmitry Baryshkov
Reviewed-by: Marijn Suijten
> ---
> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 23 ++-
> 1 file changed, 10 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> index 46d2a5c..3920efd 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -636,25 +636,22 @@ static int dpu_encoder_virt_atomic_check(
> if (ret) {
> DPU_ERROR_ENC(dpu_enc,
> "mode unsupported, phys idx %d\n", i);
> - break;
> + return ret;
> }
> }
>
> topology = dpu_encoder_get_topology(dpu_enc, dpu_kms, adj_mode,
> crtc_state);
>
> - /* Reserve dynamic resources now. */
> - if (!ret) {
> - /*
> - * Release and Allocate resources on every modeset
> - * Dont allocate when active is false.
> - */
> - if (drm_atomic_crtc_needs_modeset(crtc_state)) {
> - dpu_rm_release(global_state, drm_enc);
> + /*
> + * Release and Allocate resources on every modeset
> + * Dont allocate when active is false.
> + */
> + if (drm_atomic_crtc_needs_modeset(crtc_state)) {
> + dpu_rm_release(global_state, drm_enc);
>
> - if (!crtc_state->active_changed || crtc_state->active)
> - ret = dpu_rm_reserve(_kms->rm, global_state,
> - drm_enc, crtc_state, topology);
> - }
> + if (!crtc_state->active_changed || crtc_state->active)
> + ret = dpu_rm_reserve(_kms->rm, global_state,
> + drm_enc, crtc_state, topology);
> }
>
> trace_dpu_enc_atomic_check_flags(DRMID(drm_enc), adj_mode->flags);
> --
> 2.7.4
>
Re: [Freedreno] [PATCH v4 2/4] drm/msm/dpu: add DSPPs into reservation upon a CTM request
On 2023-02-13 03:11:42, Kalyan Thota wrote:
> Add DSPP blocks into the topology for reservation, if there
> is a CTM request for that composition.
>
> Signed-off-by: Kalyan Thota
> Reviewed-by: Dmitry Baryshkov
Reviewed-by: Marijn Suijten
> ---
> Changes in v1:
> - Minor nits (Dmitry)
>
> Changes in v2:
> - Populate DSPPs into the reservation only if CTM is requested (Dmitry)
> ---
> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 15 ++-
> 1 file changed, 6 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> index 9c6817b..46d2a5c 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -545,7 +545,8 @@ bool dpu_encoder_use_dsc_merge(struct drm_encoder
> *drm_enc)
> static struct msm_display_topology dpu_encoder_get_topology(
> struct dpu_encoder_virt *dpu_enc,
> struct dpu_kms *dpu_kms,
> - struct drm_display_mode *mode)
> + struct drm_display_mode *mode,
> + struct drm_crtc_state *crtc_state)
> {
> struct msm_display_topology topology = {0};
> int i, intf_count = 0;
> @@ -563,8 +564,7 @@ static struct msm_display_topology
> dpu_encoder_get_topology(
>* 1 LM, 1 INTF
>* 2 LM, 1 INTF (stream merge to support high resolution interfaces)
>*
> - * Adding color blocks only to primary interface if available in
> - * sufficient number
> + * Add dspps to the reservation requirements if ctm is requested
>*/
> if (intf_count == 2)
> topology.num_lm = 2;
> @@ -573,11 +573,8 @@ static struct msm_display_topology
> dpu_encoder_get_topology(
> else
> topology.num_lm = (mode->hdisplay > MAX_HDISPLAY_SPLIT) ? 2 : 1;
>
> - if (dpu_enc->disp_info.intf_type == DRM_MODE_ENCODER_DSI) {
> - if (dpu_kms->catalog->dspp &&
> - (dpu_kms->catalog->dspp_count >= topology.num_lm))
> - topology.num_dspp = topology.num_lm;
> - }
> + if (crtc_state->ctm)
> + topology.num_dspp = topology.num_lm;
>
> topology.num_enc = 0;
> topology.num_intf = intf_count;
> @@ -643,7 +640,7 @@ static int dpu_encoder_virt_atomic_check(
> }
> }
>
> - topology = dpu_encoder_get_topology(dpu_enc, dpu_kms, adj_mode);
> + topology = dpu_encoder_get_topology(dpu_enc, dpu_kms, adj_mode,
> crtc_state);
>
> /* Reserve dynamic resources now. */
> if (!ret) {
> --
> 2.7.4
>
Re: [Freedreno] [RFC PATCH 2/2] drm/msm/dp: enable pm_runtime support for dp driver
>> diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c
>> b/drivers/gpu/drm/msm/dp/dp_aux.c
>[..]
>> +static int dp_runtime_resume(struct device *dev) {
>> +struct platform_device *pdev = to_platform_device(dev);
>> +struct msm_dp *dp_display = platform_get_drvdata(pdev);
>> +struct dp_display_private *dp;
>> +
>> +dp = container_of(dp_display, struct dp_display_private, dp_display);
>> +dp_display_host_init(dp);
>> +
>> +if (dp->dp_display.is_edp) {
>> +dp_display_host_phy_init(dp);
>> +} else {
>> +dp_catalog_hpd_config_intr(dp->catalog,
>> +DP_DP_HPD_PLUG_INT_MASK |
>> +DP_DP_HPD_UNPLUG_INT_MASK,
>> +true);
>
>I believe this is backwards.
>
>Only in the event that there's no "downstream" HPD handler should we use
>the internal HPD. This is signalled by the DRM framework by a call to
>dp_bridge_hpd_enable(). So we should use that to enable/disable the
>internal HPD handler.
>
>When this happens, we have a reason for keeping power on; i.e. call
>pm_runtime_get(). Once we have power/clocking, we'd call
>dp_catalog_hpd_config_intr(), from dp_bridge_hpd_enable().
>
>
>In the case that the internal HPD handling is not use,
>dp_bridge_hpd_enable() will not be called, instead once the downstream hpd
>handler switches state dp_bridge_hpd_notify() will be invoked.
>
>In this case, we need the DP controller to be powered/clocked between
>connector_status_connected and connector_status_disconnected.
>
>
>I believe this should allow the DP controller(s) to stay powered down in the
>case where we have external HPD handling (e.g. USB Type-C or gpio-based
>dp-connector).
>
>Regards,
>Bjorn
I agree with the approach. I am moving my dev to msm-next. Will make the
changes according to the HPD handling and repost
Thank you,
Sankeerth
Re: [Freedreno] [RFC PATCH 0/2] drm/msm/dp: refactor the msm dp driver resources
>> The DP driver resources are currently enabled and disabled directly based >on code flow. >> As mentioned in bug 230631602, we want to do the following: > >private bug tracker > Will remove the reference for this. >> >> 1) Refactor the dp/edp parsing code to move it to probe (it is currently done >in bind). > >This is good. I'd suggest splitting this into smaller chunks. First, move all >resource binding, then move the actual dp_aux handling. It would be easier to >review it this way. > Okay, will move the resource binding patch first. >> 2) Then bind all the power resources needed for AUX in pm_runtime_ops. >> >> 3) Handle EPROBE_DEFER cases of the panel-eDP aux device. > >This is not handled properly. The eDP aux probing is asynchronous, so you >should move the second stage into the done_probing() part, rather than >relying on the -EPROBE_DEFER. There can be cases, when the panel driver is >not available at the DP's probe time. In such cases we should leave the DP >driver probed, then wait for the panel before binding the component. > Okay, I will explore this. >> 4) Verify DP functionality is unaffected. >> >> These code changes will parse the resources and get the edp panel during >probe. >> All the necessary resources required for the aux transactions are moved to >pm_runtime ops. >> They are enabled or disabled via get/put sync functions. >> >> This is a RFC to verify with the community if the approach we are taking is >correct. >> >> https://partnerissuetracker.corp.google.com/issues/230631602 > >This link is useless, since its contents are not public. > Okay, I will remove it. >> >> Sankeerth Billakanti (2): >> drm/msm/dp: enumerate edp panel during driver probe >> drm/msm/dp: enable pm_runtime support for dp driver >> >> drivers/gpu/drm/msm/dp/dp_aux.c | 155 +-- >> drivers/gpu/drm/msm/dp/dp_catalog.c | 12 ++ >> drivers/gpu/drm/msm/dp/dp_catalog.h | 1 + >> drivers/gpu/drm/msm/dp/dp_display.c | 185 ++-- >> drivers/gpu/drm/msm/dp/dp_power.c | 7 -- >> 5 files changed, 250 insertions(+), 110 deletions(-) >> >> -- >> 2.39.0 >> > > >-- >With best wishes >Dmitry
Re: [Freedreno] [RFC PATCH 1/2] drm/msm/dp: enumerate edp panel during driver probe
>>
>> The eDP panel is identified and enumerated during probe of the
>> panel-edp driver. The current DP driver triggers this panel-edp driver
>> probe while getting the panel-bridge associated with the eDP panel
>> from the platform driver bind. If the panel-edp probe is deferred,
>> then the whole bunch of MDSS parent and child drivers have to defer and
>roll back.
>
>No, MDSS has not been rolled back since 5.19. Please shift your development
>on top of the current msm-next.
>
Okay, I will move to the msm-next tip.
>>
>> So, we want to move the panel enumeration from bind to probe so that
>> the probe defer is easier to handle and also both the drivers appear
>> consistent in panel enumeration. This change moves the DP driver
>> panel-bridge enumeration to probe.
>>
>> Signed-off-by: Sankeerth Billakanti
>> ---
>> drivers/gpu/drm/msm/dp/dp_aux.c | 149
>++--
>> drivers/gpu/drm/msm/dp/dp_catalog.c | 12 +++
>> drivers/gpu/drm/msm/dp/dp_catalog.h | 1 +
>> drivers/gpu/drm/msm/dp/dp_display.c | 80 ++-
>> 4 files changed, 182 insertions(+), 60 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c
>> b/drivers/gpu/drm/msm/dp/dp_aux.c index cc3efed593aa..5da95dfdeede
>> 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_aux.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_aux.c
>> @@ -110,7 +110,7 @@ static ssize_t dp_aux_write(struct dp_aux_private
>> *aux, }
>>
>> static ssize_t dp_aux_cmd_fifo_tx(struct dp_aux_private *aux,
>> - struct drm_dp_aux_msg *msg)
>> + struct drm_dp_aux_msg *msg, bool poll)
>
>What is the reason for working in polled mode rather than always using the
>interrupts?
>
The mdss interrupts will be enabled and can be used after msm_irq_install from
msm_drm_bind.
We want to perform aux transactions during probe. The aux data transmission is
followed by an
interrupt to indicate successful transmission status and reply information.
As interrupts are not enabled, I took this polling approach for aux interrupts
during probe.
>> {
>> ssize_t ret;
>> unsigned long time_left;
>> @@ -121,10 +121,16 @@ static ssize_t dp_aux_cmd_fifo_tx(struct
>dp_aux_private *aux,
>> if (ret < 0)
>> return ret;
>>
>> - time_left = wait_for_completion_timeout(>comp,
>> + if (!poll) {
>> + time_left = wait_for_completion_timeout(>comp,
>> msecs_to_jiffies(250));
>> - if (!time_left)
>> - return -ETIMEDOUT;
>> + if (!time_left)
>> + return -ETIMEDOUT;
>> + } else {
>> + ret = dp_catalog_aux_poll_and_get_hw_interrupt(aux->catalog);
>> + if (!ret)
>> + dp_aux_isr(>dp_aux);
>> + }
>>
>> return ret;
>> }
>> @@ -238,7 +244,7 @@ static void
>dp_aux_update_offset_and_segment(struct dp_aux_private *aux,
>> */
>> static void dp_aux_transfer_helper(struct dp_aux_private *aux,
>>struct drm_dp_aux_msg *input_msg,
>> - bool send_seg)
>> + bool send_seg, bool poll)
>> {
>> struct drm_dp_aux_msg helper_msg;
>> u32 message_size = 0x10;
>> @@ -278,7 +284,7 @@ static void dp_aux_transfer_helper(struct
>dp_aux_private *aux,
>> helper_msg.address = segment_address;
>> helper_msg.buffer = >segment;
>> helper_msg.size = 1;
>> - dp_aux_cmd_fifo_tx(aux, _msg);
>> + dp_aux_cmd_fifo_tx(aux, _msg, poll);
>> }
>>
>> /*
>> @@ -292,7 +298,7 @@ static void dp_aux_transfer_helper(struct
>dp_aux_private *aux,
>> helper_msg.address = input_msg->address;
>> helper_msg.buffer = >offset;
>> helper_msg.size = 1;
>> - dp_aux_cmd_fifo_tx(aux, _msg);
>> + dp_aux_cmd_fifo_tx(aux, _msg, poll);
>>
>> end:
>> aux->offset += message_size;
>> @@ -300,6 +306,122 @@ static void dp_aux_transfer_helper(struct
>dp_aux_private *aux,
>> aux->segment = 0x0; /* reset segment at end of block
>> */ }
>>
>> +/*
>> + * This function does the real job to process an AUX transaction.
>> + * It will call aux_reset() function to reset the AUX channel,
>> + * if the waiting is timeout.
>> + */
>> +static ssize_t dp_aux_transfer_init(struct drm_dp_aux *dp_aux,
>> + struct drm_dp_aux_msg *msg) {
>> + ssize_t ret;
>> + int const aux_cmd_native_max = 16;
>> + int const aux_cmd_i2c_max = 128;
>> + struct dp_aux_private *aux;
>> +
>> + aux = container_of(dp_aux, struct dp_aux_private, dp_aux);
>> +
>> + aux->native = msg->request & (DP_AUX_NATIVE_WRITE &
>> + DP_AUX_NATIVE_READ);
>> +
>> + /* Ignore address only message */
>> + if (msg->size == 0 || !msg->buffer) {
>> + msg->reply =
