Re: [RFC] drm/bridge: adv7533: remove dynamic lane switching from adv7533 bridge

[Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>]

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@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)

--
With best wishes
Dmitry