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@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..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)
--
Regards,
Laurent Pinchart
