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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux