[Freedreno] [PATCH] drm/bridge: adv7533: remove dynamic lane switching from adv7533 bridge

Abhinav Kumar quic_abhinavk at quicinc.com
Tue Oct 11 21:11:57 UTC 2022



On 10/7/2022 6:56 AM, Robert Foss wrote:
> On Thu, 6 Oct 2022 at 17:07, Abhinav Kumar <quic_abhinavk at quicinc.com> wrote:
>>
>> Hi Robert
>>
>> Thanks for the review.
>>
>> On 10/4/2022 8:55 AM, Robert Foss wrote:
>>> On Mon, 29 Aug 2022 at 20:23, Abhinav Kumar <quic_abhinavk at quicinc.com> wrote:
>>>>
>>>> adv7533 bridge tries to dynamically switch lanes based on the
>>>> mode by detaching and attaching the mipi dsi device.
>>>>
>>>> This approach is incorrect because this method of dynamic switch of
>>>> detaching and attaching the mipi dsi device also results in removing
>>>> and adding the component which is not necessary.
>>>>
>>>> This approach is also prone to deadlocks. So for example, on the
>>>> db410c whenever this path is executed with lockdep enabled,
>>>> this results in a deadlock due to below ordering of locks.
>>>>
>>>> -> #1 (crtc_ww_class_acquire){+.+.}-{0:0}:
>>>>           lock_acquire+0x6c/0x90
>>>>           drm_modeset_acquire_init+0xf4/0x150
>>>>           drmm_mode_config_init+0x220/0x770
>>>>           msm_drm_bind+0x13c/0x654
>>>>           try_to_bring_up_aggregate_device+0x164/0x1d0
>>>>           __component_add+0xa8/0x174
>>>>           component_add+0x18/0x2c
>>>>           dsi_dev_attach+0x24/0x30
>>>>           dsi_host_attach+0x98/0x14c
>>>>           devm_mipi_dsi_attach+0x38/0xb0
>>>>           adv7533_attach_dsi+0x8c/0x110
>>>>           adv7511_probe+0x5a0/0x930
>>>>           i2c_device_probe+0x30c/0x350
>>>>           really_probe.part.0+0x9c/0x2b0
>>>>           __driver_probe_device+0x98/0x144
>>>>           driver_probe_device+0xac/0x14c
>>>>           __device_attach_driver+0xbc/0x124
>>>>           bus_for_each_drv+0x78/0xd0
>>>>           __device_attach+0xa8/0x1c0
>>>>           device_initial_probe+0x18/0x24
>>>>           bus_probe_device+0xa0/0xac
>>>>           deferred_probe_work_func+0x90/0xd0
>>>>           process_one_work+0x28c/0x6b0
>>>>           worker_thread+0x240/0x444
>>>>           kthread+0x110/0x114
>>>>           ret_from_fork+0x10/0x20
>>>>
>>>> -> #0 (component_mutex){+.+.}-{3:3}:
>>>>           __lock_acquire+0x1280/0x20ac
>>>>           lock_acquire.part.0+0xe0/0x230
>>>>           lock_acquire+0x6c/0x90
>>>>           __mutex_lock+0x84/0x400
>>>>           mutex_lock_nested+0x3c/0x70
>>>>           component_del+0x34/0x170
>>>>           dsi_dev_detach+0x24/0x30
>>>>           dsi_host_detach+0x20/0x64
>>>>           mipi_dsi_detach+0x2c/0x40
>>>>           adv7533_mode_set+0x64/0x90
>>>>           adv7511_bridge_mode_set+0x210/0x214
>>>>           drm_bridge_chain_mode_set+0x5c/0x84
>>>>           crtc_set_mode+0x18c/0x1dc
>>>>           drm_atomic_helper_commit_modeset_disables+0x40/0x50
>>>>           msm_atomic_commit_tail+0x1d0/0x6e0
>>>>           commit_tail+0xa4/0x180
>>>>           drm_atomic_helper_commit+0x178/0x3b0
>>>>           drm_atomic_commit+0xa4/0xe0
>>>>           drm_client_modeset_commit_atomic+0x228/0x284
>>>>           drm_client_modeset_commit_locked+0x64/0x1d0
>>>>           drm_client_modeset_commit+0x34/0x60
>>>>           drm_fb_helper_lastclose+0x74/0xcc
>>>>           drm_lastclose+0x3c/0x80
>>>>           drm_release+0xfc/0x114
>>>>           __fput+0x70/0x224
>>>>           ____fput+0x14/0x20
>>>>           task_work_run+0x88/0x1a0
>>>>           do_exit+0x350/0xa50
>>>>           do_group_exit+0x38/0xa4
>>>>           __wake_up_parent+0x0/0x34
>>>>           invoke_syscall+0x48/0x114
>>>>           el0_svc_common.constprop.0+0x60/0x11c
>>>>           do_el0_svc+0x30/0xc0
>>>>           el0_svc+0x58/0x100
>>>>           el0t_64_sync_handler+0x1b0/0x1bc
>>>>           el0t_64_sync+0x18c/0x190
>>>>
>>>> Due to above reasons, remove the dynamic lane switching
>>>> code from adv7533 bridge chip and filter out the modes
>>>> which would need different number of lanes as compared
>>>> to the initialization time using the mode_valid callback.
>>>>
>>>> This can be potentially re-introduced by using the pre_enable()
>>>> callback but this needs to be evaluated first whether such an
>>>> approach will work so this will be done with a separate change.
>>>>
>>>> changes since RFC:
>>>>           - Fix commit text and add TODO comment
>>>>
>>>> Fixes: 62b2f026cd8e ("drm/bridge: adv7533: Change number of DSI lanes dynamically")
>>>> Closes: https://gitlab.freedesktop.org/drm/msm/-/issues/16
>>>> Suggested-by: Dmitry Baryshkov <dmitry.baryshkov at linaro.org>
>>>> Signed-off-by: Abhinav Kumar <quic_abhinavk at quicinc.com>
>>>> ---
>>>>    drivers/gpu/drm/bridge/adv7511/adv7511.h     |  3 ++-
>>>>    drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 18 ++++++++++++++----
>>>>    drivers/gpu/drm/bridge/adv7511/adv7533.c     | 25 +++++++++++++------------
>>>>    3 files changed, 29 insertions(+), 17 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511.h b/drivers/gpu/drm/bridge/adv7511/adv7511.h
>>>> index 9e3bb8a8ee40..0a7cec80b75d 100644
>>>> --- a/drivers/gpu/drm/bridge/adv7511/adv7511.h
>>>> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511.h
>>>> @@ -417,7 +417,8 @@ static inline int adv7511_cec_init(struct device *dev, struct adv7511 *adv7511)
>>>>
>>>>    void adv7533_dsi_power_on(struct adv7511 *adv);
>>>>    void adv7533_dsi_power_off(struct adv7511 *adv);
>>>> -void adv7533_mode_set(struct adv7511 *adv, const struct drm_display_mode *mode);
>>>> +enum drm_mode_status adv7533_mode_valid(struct adv7511 *adv,
>>>> +               const struct drm_display_mode *mode);
>>>>    int adv7533_patch_registers(struct adv7511 *adv);
>>>>    int adv7533_patch_cec_registers(struct adv7511 *adv);
>>>>    int adv7533_attach_dsi(struct adv7511 *adv);
>>>> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
>>>> index 5bb9300040dd..1115ef9be83c 100644
>>>> --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
>>>> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
>>>> @@ -697,7 +697,7 @@ adv7511_detect(struct adv7511 *adv7511, struct drm_connector *connector)
>>>>    }
>>>>
>>>>    static enum drm_mode_status adv7511_mode_valid(struct adv7511 *adv7511,
>>>> -                             struct drm_display_mode *mode)
>>>> +                             const struct drm_display_mode *mode)
>>>>    {
>>>>           if (mode->clock > 165000)
>>>>                   return MODE_CLOCK_HIGH;
>>>> @@ -791,9 +791,6 @@ static void adv7511_mode_set(struct adv7511 *adv7511,
>>>>           regmap_update_bits(adv7511->regmap, 0x17,
>>>>                   0x60, (vsync_polarity << 6) | (hsync_polarity << 5));
>>>>
>>>> -       if (adv7511->type == ADV7533 || adv7511->type == ADV7535)
>>>> -               adv7533_mode_set(adv7511, adj_mode);
>>>> -
>>>>           drm_mode_copy(&adv7511->curr_mode, adj_mode);
>>>>
>>>>           /*
>>>> @@ -913,6 +910,18 @@ static void adv7511_bridge_mode_set(struct drm_bridge *bridge,
>>>>           adv7511_mode_set(adv, mode, adj_mode);
>>>>    }
>>>>
>>>> +static enum drm_mode_status adv7511_bridge_mode_valid(struct drm_bridge *bridge,
>>>> +               const struct drm_display_info *info,
>>>> +               const struct drm_display_mode *mode)
>>>> +{
>>>> +       struct adv7511 *adv = bridge_to_adv7511(bridge);
>>>> +
>>>> +       if (adv->type == ADV7533 || adv->type == ADV7535)
>>>> +               return adv7533_mode_valid(adv, mode);
>>>> +       else
>>>> +               return adv7511_mode_valid(adv, mode);
>>>> +}
>>>> +
>>>>    static int adv7511_bridge_attach(struct drm_bridge *bridge,
>>>>                                    enum drm_bridge_attach_flags flags)
>>>>    {
>>>> @@ -960,6 +969,7 @@ static const struct drm_bridge_funcs adv7511_bridge_funcs = {
>>>>           .enable = adv7511_bridge_enable,
>>>>           .disable = adv7511_bridge_disable,
>>>>           .mode_set = adv7511_bridge_mode_set,
>>>> +       .mode_valid = adv7511_bridge_mode_valid,
>>>>           .attach = adv7511_bridge_attach,
>>>>           .detect = adv7511_bridge_detect,
>>>>           .get_edid = adv7511_bridge_get_edid,
>>>> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7533.c b/drivers/gpu/drm/bridge/adv7511/adv7533.c
>>>> index ef6270806d1d..5f590abd6403 100644
>>>> --- a/drivers/gpu/drm/bridge/adv7511/adv7533.c
>>>> +++ b/drivers/gpu/drm/bridge/adv7511/adv7533.c
>>>> @@ -100,26 +100,27 @@ void adv7533_dsi_power_off(struct adv7511 *adv)
>>>>           regmap_write(adv->regmap_cec, 0x27, 0x0b);
>>>>    }
>>>>
>>>> -void adv7533_mode_set(struct adv7511 *adv, const struct drm_display_mode *mode)
>>>> +enum drm_mode_status adv7533_mode_valid(struct adv7511 *adv,
>>>> +               const struct drm_display_mode *mode)
>>>>    {
>>>> +       int lanes;
>>>>           struct mipi_dsi_device *dsi = adv->dsi;
>>>> -       int lanes, ret;
>>>> -
>>>> -       if (adv->num_dsi_lanes != 4)
>>>> -               return;
>>>>
>>>>           if (mode->clock > 80000)
>>>>                   lanes = 4;
>>>>           else
>>>>                   lanes = 3;
>>>>
>>>> -       if (lanes != dsi->lanes) {
>>>> -               mipi_dsi_detach(dsi);
>>>> -               dsi->lanes = lanes;
>>>> -               ret = mipi_dsi_attach(dsi);
>>>> -               if (ret)
>>>> -                       dev_err(&dsi->dev, "failed to change host lanes\n");
>>>> -       }
>>>> +       /*
>>>> +        * TODO: add support for dynamic switching of lanes
>>>> +        * by using the bridge pre_enable() op . Till then filter
>>>> +        * out the modes which shall need different number of lanes
>>>> +        * than what was configured in the device tree.
>>>> +        */
>>>> +       if (lanes != dsi->lanes)
>>>> +               return MODE_BAD;
>>>> +
>>>> +       return MODE_OK;
>>>>    }
>>>>
>>>>    int adv7533_patch_registers(struct adv7511 *adv)
>>>> --
>>>> 2.7.4
>>>>
>>>
>>> This patch has some checkpatch --style warnings, with those fixed feel
>>> free to add my r-b.
>>
>> I checked just now. I dont see any checkpatch errors on my end.
>> I am not sure if we are running different versions of checkpatch but I
>> am using the one at the tip of msm-next.
>>
>> https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/scripts/checkpatch.pl
> 
> Ah, sorry. I made a typo in my above email. '--strict' not '--style'.
> 
> checkpatch --strict

Thanks , I wasnt running with --strict.

I have fixed this and posted v2 with your R-b.

> 
>>
>>>
>>> Reviewed-by: Robert Foss <robert.foss at linaro.org>


More information about the dri-devel mailing list