| Message ID | 1660005330-12369-1-git-send-email-quic_abhinavk@quicinc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] drm/bridge: adv7533: remove dynamic lane switching from adv7533 bridge | expand |
Hi Abhinav, Thank you for the patch. On Mon, Aug 08, 2022 at 05:35:30PM -0700, Abhinav Kumar 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 as per the DSI spec the > number of lanes is fixed at the time of system design or initial > configuration and may not change dynamically. Is that really so ? The number of lanes connected on the board is certainlyset at design time, but a lower number of lanes can be used at runtime. It shouldn't change dynamically while the display is on, but it could change at mode set time. > In addition 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. Yes, that doesn't look good, and the .mode_valid() operation is definitely not the right point where to set the number of lanes. > 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. > > Suggested-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > Signed-off-by: Abhinav Kumar <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 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..4a6d45edf431 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"); > - } > + /* > + * number of lanes cannot be changed after initialization > + * as per section 6.1 of the DSI specification. Hence 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)
Hi Laurent Thanks for the response. On 8/9/2022 12:40 PM, Laurent Pinchart wrote: > Hi Abhinav, > > Thank you for the patch. > > On Mon, Aug 08, 2022 at 05:35:30PM -0700, Abhinav Kumar 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 as per the DSI spec the >> number of lanes is fixed at the time of system design or initial >> configuration and may not change dynamically. > > Is that really so ? The number of lanes connected on the board is > certainlyset at design time, but a lower number of lanes can be used at > runtime. It shouldn't change dynamically while the display is on, but it > could change at mode set time. So as per the spec it says this: "The number of Lanes used shall be a static parameter. It shall be fixed at the time of system design or initial configuration and may not change dynamically. Typically, the peripheral’s bandwidth requirement and its corresponding Lane configuration establishes the number of Lanes used in a system." But I do agree with you that this does not prohibit us from changing the lanes during mode_set time because display might have been off. Although, I really did not see any other bridges doing it this way. At the same time, detach() and attach() cycle seems the incorrect way to do it here. We did think of another approach of having a new mipi_dsi_op to reconfigure the host without the component_del()/component_add() because most of the host drivers also do component_del()/component_add() in detach()/attach(). But that would not work here either because this bridge is after the DSI controller's bridge in the chain. Hence DSI controller's modeset would happen earlier than this one. So even if we do have another op to reconfigure the host , this bridge's modeset would be too late to call that new op. It complicates things a bit, so we thought that not supporting dynamic lane switching would be the better way forward unless there is another suggestion on how to support this. > >> In addition 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. > > Yes, that doesn't look good, and the .mode_valid() operation is > definitely not the right point where to set the number of lanes. > I didnt follow this. Did you mean mode_set() is not the right point to set the number of lanes? So without this change, adv7533 changes the number of lanes during mode_set time followed by a detach/attach cycle. >> 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. >> >> Suggested-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> >> Signed-off-by: Abhinav Kumar <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 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..4a6d45edf431 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"); >> - } >> + /* >> + * number of lanes cannot be changed after initialization >> + * as per section 6.1 of the DSI specification. Hence 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) >
On 09/08/2022 22:40, Laurent Pinchart wrote: > Hi Abhinav, > > Thank you for the patch. > > On Mon, Aug 08, 2022 at 05:35:30PM -0700, Abhinav Kumar 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 as per the DSI spec the >> number of lanes is fixed at the time of system design or initial >> configuration and may not change dynamically. > > Is that really so ? The number of lanes connected on the board is > certainlyset at design time, but a lower number of lanes can be used at > runtime. It shouldn't change dynamically while the display is on, but it > could change at mode set time. I'm not sure if I interpreted the standard correctly, but I tended to have the same interpretation as Abhinav did. Anyway, even if we allow the drivers to switch the amount of lanes, this should not happen in the form of detach()/attach() cycle. The drivers use mipi_dsi_attach() as way to signal the DSI hosts that the sink device is ready. Some of DSI hosts (including MSM one) would bind components from the attach callback. If we were to support dynamically changing the amount of lanes, I would ask for additional mipi_dsi API call telling the host to switch the amount of lanes. And note that this can open the can of worms. Different hosts might have different requirements here. For example for the MSM platform the amount of lanes is programmed during bridge_pre_enable chain call, so it is possible to just set the amount of lanes following the msm_dsi_device::lanes. Other hosts might have other requirements. Thus said I'd suggest accepting this patch and maybe working on the API/support for the lane switching as a followup. > >> In addition 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. > > Yes, that doesn't look good, and the .mode_valid() operation is > definitely not the right point where to set the number of lanes. .mode_set()? > >> 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. Even without the deadlock, we are pulling the whole DRM device from beneath the DSI panel by unbinding all the components. This is not going to work correctly. >> >> -> #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. >> >> Suggested-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> >> Signed-off-by: Abhinav Kumar <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 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..4a6d45edf431 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"); >> - } >> + /* >> + * number of lanes cannot be changed after initialization >> + * as per section 6.1 of the DSI specification. Hence 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) >
On 22/08/2022 19:31, Dmitry Baryshkov wrote: > On 09/08/2022 22:40, Laurent Pinchart wrote: >> Hi Abhinav, >> >> Thank you for the patch. >> >> On Mon, Aug 08, 2022 at 05:35:30PM -0700, Abhinav Kumar 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 as per the DSI spec the >>> number of lanes is fixed at the time of system design or initial >>> configuration and may not change dynamically. >> >> Is that really so ? The number of lanes connected on the board is >> certainlyset at design time, but a lower number of lanes can be used at >> runtime. It shouldn't change dynamically while the display is on, but it >> could change at mode set time. > > I'm not sure if I interpreted the standard correctly, but I tended to > have the same interpretation as Abhinav did. > > Anyway, even if we allow the drivers to switch the amount of lanes, this > should not happen in the form of detach()/attach() cycle. The drivers > use mipi_dsi_attach() as way to signal the DSI hosts that the sink > device is ready. Some of DSI hosts (including MSM one) would bind > components from the attach callback. > > If we were to support dynamically changing the amount of lanes, I would > ask for additional mipi_dsi API call telling the host to switch the > amount of lanes. And note that this can open the can of worms. Different > hosts might have different requirements here. For example for the MSM > platform the amount of lanes is programmed during bridge_pre_enable > chain call, so it is possible to just set the amount of lanes following > the msm_dsi_device::lanes. Other hosts might have other requirements. > > Thus said I'd suggest accepting this patch and maybe working on the > API/support for the lane switching as a followup. Laurent, gracious ping for your response. > > >> >>> In addition 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. >> >> Yes, that doesn't look good, and the .mode_valid() operation is >> definitely not the right point where to set the number of lanes. > > .mode_set()? > >> >>> 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. > > Even without the deadlock, we are pulling the whole DRM device from > beneath the DSI panel by unbinding all the components. This is not going > to work correctly. > >>> >>> -> #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. >>> >>> Suggested-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> >>> Signed-off-by: Abhinav Kumar <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 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..4a6d45edf431 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"); >>> - } >>> + /* >>> + * number of lanes cannot be changed after initialization >>> + * as per section 6.1 of the DSI specification. Hence 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) >> >
Hello, On Fri, Aug 26, 2022 at 01:17:43PM +0300, Dmitry Baryshkov wrote: > On 22/08/2022 19:31, Dmitry Baryshkov wrote: > > On 09/08/2022 22:40, Laurent Pinchart wrote: > >> On Mon, Aug 08, 2022 at 05:35:30PM -0700, Abhinav Kumar 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 as per the DSI spec the > >>> number of lanes is fixed at the time of system design or initial > >>> configuration and may not change dynamically. > >> > >> Is that really so ? The number of lanes connected on the board is > >> certainlyset at design time, but a lower number of lanes can be used at > >> runtime. It shouldn't change dynamically while the display is on, but it > >> could change at mode set time. > > > > I'm not sure if I interpreted the standard correctly, but I tended to > > have the same interpretation as Abhinav did. > > > > Anyway, even if we allow the drivers to switch the amount of lanes, this > > should not happen in the form of detach()/attach() cycle. The drivers Agreed. > > use mipi_dsi_attach() as way to signal the DSI hosts that the sink > > device is ready. Some of DSI hosts (including MSM one) would bind > > components from the attach callback. > > > > If we were to support dynamically changing the amount of lanes, I would > > ask for additional mipi_dsi API call telling the host to switch the > > amount of lanes. And note that this can open the can of worms. Different > > hosts might have different requirements here. For example for the MSM > > platform the amount of lanes is programmed during bridge_pre_enable > > chain call, so it is possible to just set the amount of lanes following > > the msm_dsi_device::lanes. Other hosts might have other requirements. Conceptually, I would expect the number of effective lanes to be selected at mode set time, because it has to be done while the output is disabled. With the atomic API for bridges the .mode_set() operation is deprecated, so .pre_enable() sounds best, but there's a potential issue: the .pre_enable() operation is called from sink to source. It means that a DSI sink .pre_enable() operation would need to call a DSI host operation to set (or maybe negotiate instead of just setting a fixed value) the number of lanes first if it wants to control the sink through DCS at .pre_enable() time. We'd have to see how that fits. > > Thus said I'd suggest accepting this patch and maybe working on the > > API/support for the lane switching as a followup. I don't have a personal need for the ADV7533 so I won't really complain about any potential regression this may introduce (given that it fixes a deadlock maybe things are completely broken already and nothing can regress). I'd like to see this fixed though, doing it as a follow up is too often a way to avoid doing it at all :-) In any case, the commit message should be reworded to explain the rationale and what needs to be done. Adding a TODO or FIXME comment in the code would also help. > Laurent, gracious ping for your response. Done :-) > >>> In addition 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. > >> > >> Yes, that doesn't look good, and the .mode_valid() operation is > >> definitely not the right point where to set the number of lanes. > > > > .mode_set()? > > > >>> 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. > > > > Even without the deadlock, we are pulling the whole DRM device from > > beneath the DSI panel by unbinding all the components. This is not going > > to work correctly. > > > >>> > >>> -> #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. > >>> > >>> Suggested-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > >>> Signed-off-by: Abhinav Kumar <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 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..4a6d45edf431 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"); > >>> - } > >>> + /* > >>> + * number of lanes cannot be changed after initialization > >>> + * as per section 6.1 of the DSI specification. Hence 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)
Hi Abhinav, On Tue, Aug 09, 2022 at 02:47:32PM -0700, Abhinav Kumar wrote: > On 8/9/2022 12:40 PM, Laurent Pinchart wrote: > > On Mon, Aug 08, 2022 at 05:35:30PM -0700, Abhinav Kumar 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 as per the DSI spec the > >> number of lanes is fixed at the time of system design or initial > >> configuration and may not change dynamically. > > > > Is that really so ? The number of lanes connected on the board is > > certainlyset at design time, but a lower number of lanes can be used at > > runtime. It shouldn't change dynamically while the display is on, but it > > could change at mode set time. > > So as per the spec it says this: > > "The number of Lanes used shall be a static parameter. It shall be fixed > at the time of system design or initial configuration and may not change > dynamically. Typically, the peripheral’s bandwidth requirement and its > corresponding Lane configuration establishes the number of Lanes used in > a system." > > But I do agree with you that this does not prohibit us from changing the > lanes during mode_set time because display might have been off. Yes, I would consider mode set time as "initial configuration". > Although, I really did not see any other bridges doing it this way. > > At the same time, detach() and attach() cycle seems the incorrect way to > do it here. I fully agree. > We did think of another approach of having a new mipi_dsi_op to > reconfigure the host without the component_del()/component_add() because > most of the host drivers also do component_del()/component_add() in > detach()/attach(). Anything that avoids the usage of the component framework is likely a good idea :-) I'd really like to see it go. > But that would not work here either because this bridge is after the DSI > controller's bridge in the chain. > > Hence DSI controller's modeset would happen earlier than this one. With the atomic bridge API the .mode_set() operation is deprecated in favour of the atomic version of the enable and pre-enable operations. Pre-enable would likely be a good time to handle this. > So even if we do have another op to reconfigure the host , this bridge's > modeset would be too late to call that new op. > > It complicates things a bit, so we thought that not supporting dynamic > lane switching would be the better way forward unless there is another > suggestion on how to support this. > > >> In addition 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. > > > > Yes, that doesn't look good, and the .mode_valid() operation is > > definitely not the right point where to set the number of lanes. > > I didnt follow this. Did you mean mode_set() is not the right point to > set the number of lanes? Mode set time is conceptually the right point (but the .mode_set() operation isn't, given the above), but .mode_valid() isn't. .mode_valid() validates a mode, it should not affect the hardware configuration in any way. > So without this change, adv7533 changes the number of lanes during > mode_set time followed by a detach/attach cycle. > > >> 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. > >> > >> Suggested-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > >> Signed-off-by: Abhinav Kumar <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 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..4a6d45edf431 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"); > >> - } > >> + /* > >> + * number of lanes cannot be changed after initialization > >> + * as per section 6.1 of the DSI specification. Hence 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)
On 26/08/2022 14:55, Laurent Pinchart wrote: > Hello, > > On Fri, Aug 26, 2022 at 01:17:43PM +0300, Dmitry Baryshkov wrote: >> On 22/08/2022 19:31, Dmitry Baryshkov wrote: >>> On 09/08/2022 22:40, Laurent Pinchart wrote: >>>> On Mon, Aug 08, 2022 at 05:35:30PM -0700, Abhinav Kumar 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 as per the DSI spec the >>>>> number of lanes is fixed at the time of system design or initial >>>>> configuration and may not change dynamically. >>>> >>>> Is that really so ? The number of lanes connected on the board is >>>> certainlyset at design time, but a lower number of lanes can be used at >>>> runtime. It shouldn't change dynamically while the display is on, but it >>>> could change at mode set time. >>> >>> I'm not sure if I interpreted the standard correctly, but I tended to >>> have the same interpretation as Abhinav did. >>> >>> Anyway, even if we allow the drivers to switch the amount of lanes, this >>> should not happen in the form of detach()/attach() cycle. The drivers > > Agreed. > >>> use mipi_dsi_attach() as way to signal the DSI hosts that the sink >>> device is ready. Some of DSI hosts (including MSM one) would bind >>> components from the attach callback. >>> >>> If we were to support dynamically changing the amount of lanes, I would >>> ask for additional mipi_dsi API call telling the host to switch the >>> amount of lanes. And note that this can open the can of worms. Different >>> hosts might have different requirements here. For example for the MSM >>> platform the amount of lanes is programmed during bridge_pre_enable >>> chain call, so it is possible to just set the amount of lanes following >>> the msm_dsi_device::lanes. Other hosts might have other requirements. > > Conceptually, I would expect the number of effective lanes to be > selected at mode set time, because it has to be done while the output is > disabled. There is one tightly coupled question. The dual-DSI (or bonded-DSI) mode. Currently it is exposed as two independent DSI hosts. If we allow changing the amount of DSI lanes at runtime, bonded DSI mode would become complicated by fixing amount of lanes for each of outputs (or switching them in tight loop). And then comes the question of switching between single-DSI and bonded-DSI at runtime. > With the atomic API for bridges the .mode_set() operation is > deprecated, so .pre_enable() sounds best, but there's a potential issue: > the .pre_enable() operation is called from sink to source. It means that > a DSI sink .pre_enable() operation would need to call a DSI host > operation to set (or maybe negotiate instead of just setting a fixed > value) the number of lanes first if it wants to control the sink through > DCS at .pre_enable() time. We'd have to see how that fits. What is the fate of the patchset that implemented 'parent-first' opt-in for the drm_bridge chains? It was supposed to solve this this kind of issues. I'm asking because until it is merged some DSI hosts (e.g. the msm dsi) turn on the power in .mode_set() to allow the pre_enable() callbacks work when the DSI link is in LP11 mode. Back then I voted for the explicit mipi_dsi_power_on kind of calls, which would allow the sink bridge to prepare for the DSI powerup (e.g. by setting the amount of lanes), power up the DSI host, putting the link into LP11 and after that communicate with the sink using the DSI data lanes. > >>> Thus said I'd suggest accepting this patch and maybe working on the >>> API/support for the lane switching as a followup. > > I don't have a personal need for the ADV7533 so I won't really complain > about any potential regression this may introduce (given that it fixes a > deadlock maybe things are completely broken already and nothing can > regress). I'd like to see this fixed though, doing it as a follow up is > too often a way to avoid doing it at all :-) I don't know if this sounds like a promise, we are supporting several devices which use adv75xx (including famous dragonboard410c and less known Inforce ifc6510). So it might be (*) in our interest to restore this functionality. However as it requires adding additional API, design & negotiations might take some time. (*) might be if it limits the functionality of the device by limiting support for different modes. If not... why care then? > In any case, the commit message should be reworded to explain the > rationale and what needs to be done. Adding a TODO or FIXME comment in > the code would also help. >
Hi Dmitry, On Fri, Aug 26, 2022 at 04:52:12PM +0300, Dmitry Baryshkov wrote: > On 26/08/2022 14:55, Laurent Pinchart wrote: > > On Fri, Aug 26, 2022 at 01:17:43PM +0300, Dmitry Baryshkov wrote: > >> On 22/08/2022 19:31, Dmitry Baryshkov wrote: > >>> On 09/08/2022 22:40, Laurent Pinchart wrote: > >>>> On Mon, Aug 08, 2022 at 05:35:30PM -0700, Abhinav Kumar 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 as per the DSI spec the > >>>>> number of lanes is fixed at the time of system design or initial > >>>>> configuration and may not change dynamically. > >>>> > >>>> Is that really so ? The number of lanes connected on the board is > >>>> certainlyset at design time, but a lower number of lanes can be used at > >>>> runtime. It shouldn't change dynamically while the display is on, but it > >>>> could change at mode set time. > >>> > >>> I'm not sure if I interpreted the standard correctly, but I tended to > >>> have the same interpretation as Abhinav did. > >>> > >>> Anyway, even if we allow the drivers to switch the amount of lanes, this > >>> should not happen in the form of detach()/attach() cycle. The drivers > > > > Agreed. > > > >>> use mipi_dsi_attach() as way to signal the DSI hosts that the sink > >>> device is ready. Some of DSI hosts (including MSM one) would bind > >>> components from the attach callback. > >>> > >>> If we were to support dynamically changing the amount of lanes, I would > >>> ask for additional mipi_dsi API call telling the host to switch the > >>> amount of lanes. And note that this can open the can of worms. Different > >>> hosts might have different requirements here. For example for the MSM > >>> platform the amount of lanes is programmed during bridge_pre_enable > >>> chain call, so it is possible to just set the amount of lanes following > >>> the msm_dsi_device::lanes. Other hosts might have other requirements. > > > > Conceptually, I would expect the number of effective lanes to be > > selected at mode set time, because it has to be done while the output is > > disabled. > > There is one tightly coupled question. The dual-DSI (or bonded-DSI) > mode. Currently it is exposed as two independent DSI hosts. If we allow > changing the amount of DSI lanes at runtime, bonded DSI mode would > become complicated by fixing amount of lanes for each of outputs (or > switching them in tight loop). And then comes the question of switching > between single-DSI and bonded-DSI at runtime. Maybe we can leave dynamic selection of the number of lanes for dual-DSI out at this point ? I have no experienced with bonded DSI, is it typical to have to switch between single and bonded links at runtime (to be precise, at mode set time, not while the display is on) ? > > With the atomic API for bridges the .mode_set() operation is > > deprecated, so .pre_enable() sounds best, but there's a potential issue: > > the .pre_enable() operation is called from sink to source. It means that > > a DSI sink .pre_enable() operation would need to call a DSI host > > operation to set (or maybe negotiate instead of just setting a fixed > > value) the number of lanes first if it wants to control the sink through > > DCS at .pre_enable() time. We'd have to see how that fits. > > What is the fate of the patchset that implemented 'parent-first' opt-in > for the drm_bridge chains? It was supposed to solve this this kind of > issues. I'm asking because until it is merged some DSI hosts (e.g. the > msm dsi) turn on the power in .mode_set() to allow the pre_enable() > callbacks work when the DSI link is in LP11 mode. > > Back then I voted for the explicit mipi_dsi_power_on kind of calls, > which would allow the sink bridge to prepare for the DSI powerup (e.g. > by setting the amount of lanes), power up the DSI host, putting the link > into LP11 and after that communicate with the sink using the DSI data > lanes. A long time ago, I worked on converting the omapdrm driver to the DRM bridge API. It was using internal bridge and panel drivers with an API specific to omapdrm, and it was based on a similar principle: enabling or disabling an output went from the last component in the chain, which was the responsible for calling into its parent explicitly, with a bus-specific API. DRM bridge, on the other hand, doesn't use a recursive model but sequences the whole pipeline with a fixed order. This has led to be pre-enable/enable split, and even that isn't enough. Moving from the omapdrm model to the DRM bridge model was difficult and took lots of time and effort, and I'm now increasingly thinking the omapdrm model got it right, only too early to convince enough people. > >>> Thus said I'd suggest accepting this patch and maybe working on the > >>> API/support for the lane switching as a followup. > > > > I don't have a personal need for the ADV7533 so I won't really complain > > about any potential regression this may introduce (given that it fixes a > > deadlock maybe things are completely broken already and nothing can > > regress). I'd like to see this fixed though, doing it as a follow up is > > too often a way to avoid doing it at all :-) > > I don't know if this sounds like a promise, we are supporting several > devices which use adv75xx (including famous dragonboard410c and less > known Inforce ifc6510). So it might be (*) in our interest to restore > this functionality. However as it requires adding additional API, design > & negotiations might take some time. That's fine. > (*) might be if it limits the functionality of the device by limiting > support for different modes. If not... why care then? > > > In any case, the commit message should be reworded to explain the > > rationale and what needs to be done. Adding a TODO or FIXME comment in > > the code would also help.
Hi Laurent On 8/26/2022 4:55 AM, Laurent Pinchart wrote: > Hello, > > On Fri, Aug 26, 2022 at 01:17:43PM +0300, Dmitry Baryshkov wrote: >> On 22/08/2022 19:31, Dmitry Baryshkov wrote: >>> On 09/08/2022 22:40, Laurent Pinchart wrote: >>>> On Mon, Aug 08, 2022 at 05:35:30PM -0700, Abhinav Kumar 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 as per the DSI spec the >>>>> number of lanes is fixed at the time of system design or initial >>>>> configuration and may not change dynamically. >>>> >>>> Is that really so ? The number of lanes connected on the board is >>>> certainlyset at design time, but a lower number of lanes can be used at >>>> runtime. It shouldn't change dynamically while the display is on, but it >>>> could change at mode set time. >>> >>> I'm not sure if I interpreted the standard correctly, but I tended to >>> have the same interpretation as Abhinav did. >>> >>> Anyway, even if we allow the drivers to switch the amount of lanes, this >>> should not happen in the form of detach()/attach() cycle. The drivers > > Agreed. > >>> use mipi_dsi_attach() as way to signal the DSI hosts that the sink >>> device is ready. Some of DSI hosts (including MSM one) would bind >>> components from the attach callback. >>> >>> If we were to support dynamically changing the amount of lanes, I would >>> ask for additional mipi_dsi API call telling the host to switch the >>> amount of lanes. And note that this can open the can of worms. Different >>> hosts might have different requirements here. For example for the MSM >>> platform the amount of lanes is programmed during bridge_pre_enable >>> chain call, so it is possible to just set the amount of lanes following >>> the msm_dsi_device::lanes. Other hosts might have other requirements. > > Conceptually, I would expect the number of effective lanes to be > selected at mode set time, because it has to be done while the output is > disabled. With the atomic API for bridges the .mode_set() operation is > deprecated, so .pre_enable() sounds best, but there's a potential issue: > the .pre_enable() operation is called from sink to source. It means that > a DSI sink .pre_enable() operation would need to call a DSI host > operation to set (or maybe negotiate instead of just setting a fixed > value) the number of lanes first if it wants to control the sink through > DCS at .pre_enable() time. We'd have to see how that fits. > >>> Thus said I'd suggest accepting this patch and maybe working on the >>> API/support for the lane switching as a followup. > > I don't have a personal need for the ADV7533 so I won't really complain > about any potential regression this may introduce (given that it fixes a > deadlock maybe things are completely broken already and nothing can > regress). I'd like to see this fixed though, doing it as a follow up is > too often a way to avoid doing it at all :-) > > In any case, the commit message should be reworded to explain the > rationale and what needs to be done. Adding a TODO or FIXME comment in > the code would also help. > Yes, I have added a TODO in the code . fixed the commit text and posted this removing the RFC tag to address this deadlock issue. Thanks Abhinav >> Laurent, gracious ping for your response. > > Done :-) > >>>>> In addition 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. >>>> >>>> Yes, that doesn't look good, and the .mode_valid() operation is >>>> definitely not the right point where to set the number of lanes. >>> >>> .mode_set()? >>> >>>>> 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. >>> >>> Even without the deadlock, we are pulling the whole DRM device from >>> beneath the DSI panel by unbinding all the components. This is not going >>> to work correctly. >>> >>>>> >>>>> -> #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. >>>>> >>>>> Suggested-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> >>>>> Signed-off-by: Abhinav Kumar <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 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..4a6d45edf431 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"); >>>>> - } >>>>> + /* >>>>> + * number of lanes cannot be changed after initialization >>>>> + * as per section 6.1 of the DSI specification. Hence 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) >
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..4a6d45edf431 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"); - } + /* + * number of lanes cannot be changed after initialization + * as per section 6.1 of the DSI specification. Hence 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)
adv7533 bridge tries to dynamically switch lanes based on the mode by detaching and attaching the mipi dsi device. This approach is incorrect because as per the DSI spec the number of lanes is fixed at the time of system design or initial configuration and may not change dynamically. In addition 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. Suggested-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> Signed-off-by: Abhinav Kumar <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(-)