Hi, On 19/08/2022 04:34, Laurent Pinchart wrote:
Hi Tomi, Thank you for the patch. On Wed, Aug 17, 2022 at 04:28:03PM +0300, Tomi Valkeinen wrote:From: Tomi Valkeinen <tomi.valkeinen+renesas@xxxxxxxxxxxxxxxx> The rcar crtc depends on the clock provided from the rcar DSI bridge. When the DSI bridge is disabled, the clock is stopped, which causes the crtc disable to timeout. Also, while I have no issue with the enable, the documentation suggests to enable the DSI before the crtc so that the crtc has its clock enabled at enable time. This is also not done by the current driver. To fix this, we need to keep the DSI bridge enabled until the crtc has disabled itself, and enable the DSI bridge before crtc enables itself. Add functions (rcar_mipi_dsi_atomic_early_enable and rcar_mipi_dsi_atomic_late_disable) to the rcar DSI bridge driver which the rcar driver can use to enable/disable the DSI clock when needed. This is similar to what is already done with the rcar LVDS bridge.I had hoped we could avoid that :-( I wonder if we could instead rely on the pre_enable/post_disable bridge operations, with a custom commit tail implementation to order those before/after the CRTC enable/disable respectively. That would be pretty complex though, so probably not worth it.
I think so, especially as we already have a similar case for LVDS, and these custom calls are quite simple to implement and understand. I fear a custom commit implementation would be a much bigger maintenance burden for little, if any, benefit.
Thinking more about this, I wonder why pre_enable is not called before enabling the CRTC in the DRM atomic helpers. That would make more sense to me, but I suppose changing it would break too many things ? I think it should at least be discussed to figure out if it was a historical oversight or if there was a good reason. It's *really* not nice to poke holes through the abstraction layers like this.
Yes, I'll bring this question to #dri-devel. But I think it's better to get the issue fixed with a custom function call as done in this patch, and hope that we can do the work in a standard way in the future.
Also add a new function, rcar_mipi_dsi_stop_video(), called from rcar_mipi_dsi_atomic_enable so that the DSI TX gets disabled at the correct time, even if the clocks are still kept enabled.I think this should be split to a separate patch, possibly before this one, it addresses a separate issue.
I agree.
And, while possibly not strictly needed, clear clock related enable bits in rcar_mipi_dsi_atomic_late_disable to mirror the sequence done in rcar_mipi_dsi_startup() (via rcar_mipi_dsi_atomic_early_enable).And this too should be a separate patch, possibly bundled with rcar_mipi_dsi_stop_video().
Yep. I'll have it in a separate patch as they're somewhat different.
Have you checked in the BSP code to see if they do the same at disable time ?
No, they don't seem to do this.The steps for stopping of the video transmission is described in the doc, but there's no mention I can see of clearing those bits (or rather, making sure they are cleared before starting the next enable sequence).
Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@xxxxxxxxxxxxxxxx> --- drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 25 +++++++++ drivers/gpu/drm/rcar-du/rcar_du_drv.h | 2 + drivers/gpu/drm/rcar-du/rcar_du_encoder.c | 4 ++ drivers/gpu/drm/rcar-du/rcar_mipi_dsi.c | 63 +++++++++++++++++++++-- drivers/gpu/drm/rcar-du/rcar_mipi_dsi.h | 32 ++++++++++++ 5 files changed, 121 insertions(+), 5 deletions(-) create mode 100644 drivers/gpu/drm/rcar-du/rcar_mipi_dsi.h diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c index fd3b94649a01..51fd1d99f4e8 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c @@ -29,6 +29,7 @@ #include "rcar_du_regs.h" #include "rcar_du_vsp.h" #include "rcar_lvds.h" +#include "rcar_mipi_dsi.h"static u32 rcar_du_crtc_read(struct rcar_du_crtc *rcrtc, u32 reg){ @@ -733,6 +734,18 @@ static void rcar_du_crtc_atomic_enable(struct drm_crtc *crtc, rcar_cmm_enable(rcrtc->cmm); rcar_du_crtc_get(rcrtc);A comment here similar to the LVDS case would be useful. I would actually move this below the LVDS code, and write /* * Similarly, on V3U, ... */
Ok.
+ if ((rcdu->info->dsi_clk_mask & BIT(rcrtc->index)) && + (rstate->outputs & + (BIT(RCAR_DU_OUTPUT_DSI0) | BIT(RCAR_DU_OUTPUT_DSI1)))) { + struct drm_bridge *bridge = rcdu->dsi[rcrtc->index]; + + /* + * Enable the DSI clock output. + */ + + rcar_mipi_dsi_atomic_early_enable(bridge, state); + }I was wondering if we could merge the DSI and LVDS clock enabling code, including merging the lvds and dsi fields in rcar_du_device, but it doesn't seem it will be very clean here.
True, they are quite similar. I didn't want to mix them up as I don't have the means to test lvds, nor am I that familiar with the rcar du.
If I'm not mistaken, the difference is that LVDS clock is used for non-LVDS output, whereas here the DSI clock is used for DSI output.
+ /* * On D3/E3 the dot clock is provided by the LVDS encoder attached to * the DU channel. We need to enable its clock output explicitly if @@ -769,6 +782,18 @@ static void rcar_du_crtc_atomic_disable(struct drm_crtc *crtc, rcar_du_crtc_stop(rcrtc); rcar_du_crtc_put(rcrtc);+ if ((rcdu->info->dsi_clk_mask & BIT(rcrtc->index)) &&+ (rstate->outputs & + (BIT(RCAR_DU_OUTPUT_DSI0) | BIT(RCAR_DU_OUTPUT_DSI1)))) { + struct drm_bridge *bridge = rcdu->dsi[rcrtc->index]; + + /* + * Disable the DSI clock output. + */ + + rcar_mipi_dsi_atomic_late_disable(bridge); + } + if (rcdu->info->lvds_clk_mask & BIT(rcrtc->index) && rstate->outputs == BIT(RCAR_DU_OUTPUT_DPAD0)) { struct drm_bridge *bridge = rcdu->lvds[rcrtc->index]; diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.h b/drivers/gpu/drm/rcar-du/rcar_du_drv.h index 712389c7b3d0..5cfa2bb7ad93 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.h +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.h @@ -92,6 +92,7 @@ struct rcar_du_device_info { #define RCAR_DU_MAX_GROUPS DIV_ROUND_UP(RCAR_DU_MAX_CRTCS, 2) #define RCAR_DU_MAX_VSPS 4 #define RCAR_DU_MAX_LVDS 2 +#define RCAR_DU_MAX_DSI 2struct rcar_du_device {struct device *dev; @@ -108,6 +109,7 @@ struct rcar_du_device { struct platform_device *cmms[RCAR_DU_MAX_CRTCS]; struct rcar_du_vsp vsps[RCAR_DU_MAX_VSPS]; struct drm_bridge *lvds[RCAR_DU_MAX_LVDS]; + struct drm_bridge *dsi[RCAR_DU_MAX_DSI];struct {struct drm_property *colorkey; diff --git a/drivers/gpu/drm/rcar-du/rcar_du_encoder.c b/drivers/gpu/drm/rcar-du/rcar_du_encoder.c index 60d6be78323b..ac93e08e8af4 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_encoder.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_encoder.c @@ -84,6 +84,10 @@ int rcar_du_encoder_init(struct rcar_du_device *rcdu, if (output == RCAR_DU_OUTPUT_LVDS0 || output == RCAR_DU_OUTPUT_LVDS1) rcdu->lvds[output - RCAR_DU_OUTPUT_LVDS0] = bridge; + + if (output == RCAR_DU_OUTPUT_DSI0 || + output == RCAR_DU_OUTPUT_DSI1) + rcdu->dsi[output - RCAR_DU_OUTPUT_DSI0] = bridge; }/*diff --git a/drivers/gpu/drm/rcar-du/rcar_mipi_dsi.c b/drivers/gpu/drm/rcar-du/rcar_mipi_dsi.c index 62f7eb84ab01..1ec40e40fd08 100644 --- a/drivers/gpu/drm/rcar-du/rcar_mipi_dsi.c +++ b/drivers/gpu/drm/rcar-du/rcar_mipi_dsi.c @@ -542,6 +542,34 @@ static int rcar_mipi_dsi_start_video(struct rcar_mipi_dsi *dsi) return 0; }+static void rcar_mipi_dsi_stop_video(struct rcar_mipi_dsi *dsi)+{ + u32 status; + int ret; + + /* Disable transmission in video mode. */ + rcar_mipi_dsi_clr(dsi, TXVMCR, TXVMCR_EN_VIDEO); + + ret = read_poll_timeout(rcar_mipi_dsi_read, status, + !(status & TXVMSR_ACT), + 2000, 100000, false, dsi, TXVMSR); + if (ret < 0) { + dev_err(dsi->dev, "Failed to disable video transmission\n"); + return; + } + + /* Assert video FIFO clear. */ + rcar_mipi_dsi_set(dsi, TXVMCR, TXVMCR_VFCLR); + + ret = read_poll_timeout(rcar_mipi_dsi_read, status, + !(status & TXVMSR_VFRDY), + 2000, 100000, false, dsi, TXVMSR); + if (ret < 0) { + dev_err(dsi->dev, "Failed to assert video FIFO clear\n"); + return; + } +} + /* ----------------------------------------------------------------------------- * Bridge */ @@ -558,7 +586,22 @@ static int rcar_mipi_dsi_attach(struct drm_bridge *bridge, static void rcar_mipi_dsi_atomic_enable(struct drm_bridge *bridge, struct drm_bridge_state *old_bridge_state) { - struct drm_atomic_state *state = old_bridge_state->base.state; + struct rcar_mipi_dsi *dsi = bridge_to_rcar_mipi_dsi(bridge); + + rcar_mipi_dsi_start_video(dsi); +} + +static void rcar_mipi_dsi_atomic_disable(struct drm_bridge *bridge, + struct drm_bridge_state *old_bridge_state) +{ + struct rcar_mipi_dsi *dsi = bridge_to_rcar_mipi_dsi(bridge); + + rcar_mipi_dsi_stop_video(dsi); +} + +void rcar_mipi_dsi_atomic_early_enable(struct drm_bridge *bridge, + struct drm_atomic_state *state) +{ struct rcar_mipi_dsi *dsi = bridge_to_rcar_mipi_dsi(bridge); const struct drm_display_mode *mode; struct drm_connector *connector; @@ -586,8 +629,6 @@ static void rcar_mipi_dsi_atomic_enable(struct drm_bridge *bridge, if (ret < 0) goto err_dsi_start_hs;- rcar_mipi_dsi_start_video(dsi);- return;err_dsi_start_hs:@@ -595,15 +636,27 @@ static void rcar_mipi_dsi_atomic_enable(struct drm_bridge *bridge, err_dsi_startup: rcar_mipi_dsi_clk_disable(dsi); } +EXPORT_SYMBOL_GPL(rcar_mipi_dsi_atomic_early_enable);-static void rcar_mipi_dsi_atomic_disable(struct drm_bridge *bridge,- struct drm_bridge_state *old_bridge_state) +void rcar_mipi_dsi_atomic_late_disable(struct drm_bridge *bridge) { struct rcar_mipi_dsi *dsi = bridge_to_rcar_mipi_dsi(bridge);+ rcar_mipi_dsi_clr(dsi, VCLKEN, VCLKEN_CKEN);+ + /* Disable DOT clock */ + rcar_mipi_dsi_clr(dsi, VCLKSET, VCLKSET_CKEN); + + /* CFGCLK disable */ + rcar_mipi_dsi_clr(dsi, CFGCLKSET, CFGCLKSET_CKEN); + + /* LPCLK disable */ + rcar_mipi_dsi_clr(dsi, LPCLKSET, LPCLKSET_CKEN); + rcar_mipi_dsi_shutdown(dsi); rcar_mipi_dsi_clk_disable(dsi); } +EXPORT_SYMBOL_GPL(rcar_mipi_dsi_atomic_late_disable);static enum drm_mode_statusrcar_mipi_dsi_bridge_mode_valid(struct drm_bridge *bridge, diff --git a/drivers/gpu/drm/rcar-du/rcar_mipi_dsi.h b/drivers/gpu/drm/rcar-du/rcar_mipi_dsi.h new file mode 100644 index 000000000000..1764abf65a34 --- /dev/null +++ b/drivers/gpu/drm/rcar-du/rcar_mipi_dsi.h @@ -0,0 +1,32 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/* + * R-Car DSI Encoder + * + * Copyright (C) 2022 Renesas Electronics Corporation + * + * Contact: Tomi Valkeinen <tomi.valkeinen@xxxxxxxxxxxxxxxx> + */ + +#ifndef __RCAR_MIPI_DSI_H__ +#define __RCAR_MIPI_DSI_H__ + +struct drm_bridge; +struct drm_atomic_state;Alphabetical order please.
Ok.
+ +#if IS_ENABLED(CONFIG_DRM_RCAR_MIPI_DSI) +void rcar_mipi_dsi_atomic_early_enable(struct drm_bridge *bridge, + struct drm_atomic_state *state); +void rcar_mipi_dsi_atomic_late_disable(struct drm_bridge *bridge);It would be nice to have a naming scheme consistent with rcar_lvds.h. That would mean rcar_mipi_dsi_clk_{enable,disable}(), or renaming the LVDS functions to match whatever name would be picked here. We could name the functions pre_enable/post_disable to show what they should really be, if it wasn't for the DRM atomic helpers calling the bridge operations at the wrong time.
DSI already has rcar_mipi_dsi_clk_enable(). I'll try to figure out a suitable common naming.
Tomi
