Re: [PATCH 3/3] drm: rcar-du: fix DSI enable & disable sequence

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

 



Hi Tomi,

On Mon, Aug 22, 2022 at 12:02:06PM +0300, Tomi Valkeinen wrote:
> On 19/08/2022 04:34, Laurent Pinchart wrote:
> > 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.

That's fine. If it turns out that the result of the discussion on
#dri-devel is that this should be fixed in the DRM core, with a simple
and clear path forward, then the next version of this series could be
based on that, otherwise I'm fine with this fix.

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

I can test it, but I don't think the resulting code would be very clean
anyway.

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

That's correct. That's why I initially thought we could avoid enabling
the clock manually for DSI.

> >> +
> >>   	/*
> >>   	 * 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			2
> >>   
> >>   struct 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_status
> >>   rcar_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.

-- 
Regards,

Laurent Pinchart



[Index of Archives]     [Linux Samsung SOC]     [Linux Wireless]     [Linux Kernel]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Device Mapper]

  Powered by Linux