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