From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id D8A68ECAAA3 for ; Fri, 26 Aug 2022 11:59:36 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 5D5F810E87A; Fri, 26 Aug 2022 11:59:34 +0000 (UTC) Received: from perceval.ideasonboard.com (perceval.ideasonboard.com [213.167.242.64]) by gabe.freedesktop.org (Postfix) with ESMTPS id E549710E88D; Fri, 26 Aug 2022 11:59:28 +0000 (UTC) Received: from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi [62.78.145.57]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id 4F5C82B3; Fri, 26 Aug 2022 13:59:27 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1661515167; bh=CS5piaRjXy+DxHLV3y95syJlOsE1ZwwfLPtC1JebilU=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=lUWF/5pBONXKSORhwZRZ63v81J8IzAy+y7wkmRIf5WmdZBbKUam41CyKmCQjhUC5D 4aNdVrwGWLtZCJyqN0veGloOYtXrJEZo7WUdAs10Jyuvr1Ks9ysM/7RGcfz1f01A87 knktRFCsJTPJiG9fmvUQz2tIuoMG2gGiUeuD1KbQ= Date: Fri, 26 Aug 2022 14:59:20 +0300 From: Laurent Pinchart To: Abhinav Kumar Subject: Re: [RFC] drm/bridge: adv7533: remove dynamic lane switching from adv7533 bridge Message-ID: References: <1660005330-12369-1-git-send-email-quic_abhinavk@quicinc.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: jernej.skrabec@gmail.com, andrzej.hajda@intel.com, narmstrong@baylibre.com, airlied@linux.ie, sam@ravnborg.org, jonas@kwiboo.se, dri-devel@lists.freedesktop.org, swboyd@chromium.org, seanpaul@chromium.org, robert.foss@linaro.org, dmitry.baryshkov@linaro.org, quic_jesszhan@quicinc.com, quic_aravindh@quicinc.com, colin.king@intel.com, freedreno@lists.freedesktop.org, maxime@cerno.tech Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" 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 > >> Signed-off-by: Abhinav Kumar > >> --- > >> 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