Re: [PATCH 3/3] drm: rcar-du: fix DSI enable & disable sequence
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
> >>
> >> 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
> >> ---
> >> 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, ...
>
Re: [PATCH 3/3] drm: rcar-du: fix DSI enable & disable sequence
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
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
---
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
Re: [PATCH 3/3] drm: rcar-du: fix DSI enable & disable sequence
Hi Tomi,
Thank you for the patch.
On Wed, Aug 17, 2022 at 04:28:03PM +0300, Tomi Valkeinen wrote:
> From: Tomi Valkeinen
>
> 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
> ---
> 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 ==
