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