[PATCH v2] drm/bridge: adv7533: remove dynamic lane switching from adv7533 bridge
Abhinav Kumar
quic_abhinavk at quicinc.com
Tue Mar 14 17:13:07 UTC 2023
Thanks Dmitry and Adam.
On 3/11/2023 3:09 PM, Dmitry Baryshkov wrote:
> On 12/03/2023 00:52, Adam Ford wrote:
>> On Sat, Mar 11, 2023 at 4:42 PM Dmitry Baryshkov
>> <dmitry.baryshkov at linaro.org> wrote:
>>>
>>> On 12/03/2023 00:40, Adam Ford wrote:
>>>> On Sat, Mar 11, 2023 at 4:02 PM Dmitry Baryshkov
>>>> <dmitry.baryshkov at linaro.org> wrote:
>>>>>
>>>>> On 11/03/2023 23:53, Adam Ford wrote:
>>>>>> On Sat, Mar 11, 2023 at 3:40 PM Dmitry Baryshkov
>>>>>> <dmitry.baryshkov at linaro.org> wrote:
>>>>>>>
>>>>>>> "Hi Abhinav,
>>>>>>>
>>>>>>> On Sat, 11 Mar 2023 at 23:18, Abhinav Kumar
>>>>>>> <quic_abhinavk at quicinc.com> wrote:
>>>>>>>>
>>>>>>>> Hi Adam
>>>>>>>>
>>>>>>>> On 3/11/2023 9:28 AM, Adam Ford wrote:
>>>>>>>>> On Thu, Oct 13, 2022 at 3:39 AM Robert Foss
>>>>>>>>> <robert.foss at linaro.org> wrote:
>>>>>>>>>>
>>>>>>>>>> On Tue, 11 Oct 2022 at 23:11, 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
>>>>>>>>>>>
>>>>>>>>>>> changes in v2:
>>>>>>>>>>> - Fix checkpatch formatting errors
>>>>>>>>>>>
>>>>>>>>>>> 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>
>>>>>>>>>>> Reviewed-by: Robert Foss <robert.foss at linaro.org>
>>>>>>>>>>> Link:
>>>>>>>>>>> https://lore.kernel.org/r/1661797363-7564-1-git-send-email-quic_abhinavk@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 a031a0cd1f18..1053d185b24c 100644
>>>>>>>>>>> --- a/drivers/gpu/drm/bridge/adv7511/adv7511.h
>>>>>>>>>>> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511.h
>>>>>>>>>>> @@ -405,7 +405,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 38bf28720f3a..4bc7aac94a16 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..258c79d4dab0 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;
>>>>>>>>>
>>>>>>>>> I know this thread is a bit old, but I have an i.MX8M Mini with an
>>>>>>>>> adv7535 HDMI bridge, and I'm able to display video on a 4-lane
>>>>>>>>> interface with a clock as low as 27000.
>>>>>>>>>
>>>>>>>>
>>>>>>>> Looks like this condition is not tracking how low the bridge can
>>>>>>>> support
>>>>>>>> with 4 lanes but how high you can support with 3 lanes. Thats
>>>>>>>> the issue.
>>>>>>>>
>>>>>>>> This condition has been this way since the original bridge driver:
>>>>>>>>
>>>>>>>> drm/bridge: adv7533: Change number of DSI lanes dynamically
>>>>>>>>
>>>>>>>> Yes, its possible that with 4 lanes you can support a lower rate
>>>>>>>> but
>>>>>>>> with 3 lanes you cannot support a higher one. So that needs to
>>>>>>>> be handled.
>>>>>>>>
>>>>>>>> The extra conditional's intention was to make sure that if you
>>>>>>>> try to
>>>>>>>> switch to a mode which has a different number of lanes than what
>>>>>>>> was set
>>>>>>>> in the device tree (basically dynamic mode switch) then fail that.
>>>>>>>>
>>>>>>>> So perhaps this condition can be modified to :
>>>>>>>>
>>>>>>>> if (num_dsi_lanes < 4 && mode->clock > 80000)
>>>>>>>> return MODE_BAD;
>>>>>>>>
>>>>>>>> I think that should fix your issue. You can set the number of
>>>>>>>> lanes to 4
>>>>>>>> for your board if it supports that using the DT node adi,dsi-lanes.
>>>>>>>
>>>>>>
>>>>>> Thanks to both of you for the ideas.
>>>>>>
>>>>>>> I went on and looked into the adv7533 datasheet. Strangely enough
>>>>>>> these conditions do not correspond to the limitations from the
>>>>>>> datasheet:
>>>>>>> - "pixel clocks of up to 80 MHz",
>>>>>>> - "each [DSI lane] running up to 800 Mbps"
>>>>>>>
>>>>>>> So probably the conditions should be as following:
>>>>>>>
>>>>>>> if (mode->clock > 80000)
>>>>>>> return MODE_CLOCK_HIGH;
>>>>>>
>>>>>> This is consistent with what is happening on the 7511.
>>>>>>
>>>>>>>
>>>>>>> if (mode->clock * bpp > 800000 * adv->num_dsi_lanes)
>>>>>>> return MODE_CLOCK_HIGH;
>>>>>>>
>>>>>>> See
>>>>>>> https://www.analog.com/media/en/technical-documentation/data-sheets/adv7533.pdf
>>>>>>>
Yes, even i am unclear on the origins of the lane conditions. It has
been this way since the bridge originally intended to add dynamic mode
switch. But I am fine with getting rid of them and replacing it with
whats actually there in the spec (thanks for locating it).
>>>>>>
>>>>>> These numbers would need to be changed for the 7535, the device I am
>>>>>> using. So I'll submit a patch which checks these with different max
>>>>>> values depending on whether or not the device is 7533 or 7535.
>>>>>
>>>>> For adv7535 the max pixel clock seems to 148.5 MHz, max lane clock is
>>>>> 891 MHz.
>>>>>
>>>>> I'm slightly confused about adv7511, whose datasheet contains just a
>>>>> single number of '225 MHz'.
>>>>
>>>> This is what I did:
>>>>
>>>> u8 bpp = mipi_dsi_pixel_format_to_bpp(dsi->format);
>>>> ...
>>>>
>>>> /* Check max clock for either 7533 or 7535 */
>>>> if (mode->clock > (adv->type == ADV7533 ? 80000 : 148500))
>>>> return MODE_CLOCK_HIGH;
>>>>
>>>> /* Check max clock for each lane */
>>>> max_lane_freq = (adv->type == ADV7533 ? 800000 : 891000);
>>>>
>>>> if (mode->clock * bpp > max_lane_freq * adv->num_dsi_lanes)
>>>> return MODE_CLOCK_HIGH;
>>>>
>>>> Does that look OK to everyone?
>>>>
>>>> If so, I'll push a formal patch with a fixes tag
>>>
>>> LGTM. You might want to split this to two separate functions (7533 vs
>>> 7535), but it's up to your taste.
>>
>> I intentionally wanted to keep them together since the 7535 uses the
>> code from the 7533, and I thought it was simpler to do the check in
>> one place instead of having two sets of nearly identical checks.
>> I'm just finishing the clean-up, so the patch should go out momentarily.
>
> Ack
>
Thanks Adam.
>>
>> adam
>>>
>>> .
>>>>
>>>> adam
>>>>>
>>>>>>
>>>>>> adam
>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> - 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;
>>>>>>>>>>> +
>>>>>>>>>
>>>>>>>>> My board doesn't currently support dynamic switching, but I'd
>>>>>>>>> like to
>>>>>>>>> keep 4-lanes all the time. However, this return eliminates
>>>>>>>>> several
>>>>>>>>> resolutions that I can successfully display.
>>>>>>>>> I'd like to eliminate this error, so it works on the imx8m
>>>>>>>>> mini/nano/plus, but I am not sure the best approach without
>>>>>>>>> breaking
>>>>>>>>> someone else's board.
>>>>>>>>> > I was thinking I could add a flag to disable dynamic
>>>>>>>>> switching. If
>>>>>>>>> that flag is set, we'd return MODE_OK here.
>>>>>>>>>
>>>>>>>>> Does anyone have any suggestions on an apporach?
>>>>>>>>>
>>>>>>>>> adam
>>>>>>>>>
>>>>>>>>
>>>>>>>> I am thinking of this solution
>>>>>>>>
>>>>>>>> if (num_dsi_lanes < 4 && mode->clock > 80000)
>>>>>>>> return MODE_BAD;
>>>>>>>>
>>>>>>>> So lets say some user tried to set number of lanes to 3 but
>>>>>>>> tries a mode
>>>>>>>> needing > 80000 clock then fail.
>>>>>>>>
>>>>>>>> That way we still dont support dynamic mode switch but also dont
>>>>>>>> limit
>>>>>>>> the modes which can be supported with 4 lanes.
>>>>>>>>
>>>>>>>>>
>>>>>>>>>>> + return MODE_OK;
>>>>>>>>>>> }
>>>>>>>>>>>
>>>>>>>>>>> int adv7533_patch_registers(struct adv7511 *adv)
>>>>>>>>>>> --
>>>>>>>>>>> 2.7.4
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Applied to drm-misc-next.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> --
>>>>>>> With best wishes
>>>>>>> Dmitry
>>>>>
>>>>> --
>>>>> With best wishes
>>>>> Dmitry
>>>>>
>>>
>>> --
>>> With best wishes
>>> Dmitry
>>>
>
More information about the dri-devel
mailing list