| Message ID | 20220817132803.85373-1-tomi.valkeinen@ideasonboard.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/3] drm: rcar-du: remove unnecessary include | expand |
Hi Tomi, Thank you for the patch. On Wed, Aug 17, 2022 at 04:28:01PM +0300, Tomi Valkeinen wrote: > From: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com> > > rcar_du_regs.h is not needed by rcar_du_drv.c so drop the include. > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > drivers/gpu/drm/rcar-du/rcar_du_drv.c | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.c b/drivers/gpu/drm/rcar-du/rcar_du_drv.c > index 00ac233a115e..541c202c993a 100644 > --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.c > +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.c > @@ -27,7 +27,6 @@ > > #include "rcar_du_drv.h" > #include "rcar_du_kms.h" > -#include "rcar_du_regs.h" > > /* ----------------------------------------------------------------------------- > * Device Information
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@ideasonboard.com> > > 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. 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. > 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. > 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(). Have you checked in the BSP code to see if they do the same at disable time ? > Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com> > --- > 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, ... */ > + 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. > + > /* > * 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@ideasonboard.com> > + */ > + > +#ifndef __RCAR_MIPI_DSI_H__ > +#define __RCAR_MIPI_DSI_H__ > + > +struct drm_bridge; > +struct drm_atomic_state; Alphabetical order please. > + > +#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. > +#else > +static inline 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) > +{ > +} > +#endif /* CONFIG_DRM_RCAR_MIPI_DSI */ > + > +#endif /* __RCAR_MIPI_DSI_H__ */
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@ideasonboard.com> >> >> 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@ideasonboard.com> >> --- >> 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 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@ideasonboard.com> >> + */ >> + >> +#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
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@ideasonboard.com> > >> > >> 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@ideasonboard.com> > >> --- > >> 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@ideasonboard.com> > >> + */ > >> + > >> +#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.
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.c b/drivers/gpu/drm/rcar-du/rcar_du_drv.c index 00ac233a115e..541c202c993a 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.c @@ -27,7 +27,6 @@ #include "rcar_du_drv.h" #include "rcar_du_kms.h" -#include "rcar_du_regs.h" /* ----------------------------------------------------------------------------- * Device Information