Re: [PATCH 3/3] drm: rcar-du: lvds: Fix LVDS PLL disable on D3/E3
Hi Tomi,
On Wed, Feb 15, 2023 at 08:29:47AM +0200, Tomi Valkeinen wrote:
> On 14/02/2023 02:37, Laurent Pinchart wrote:
> > On R-Car D3 and E3, the LVDS encoder provides the dot (pixel) clock to
> > the DU, regardless of whether the LVDS output is used or not. When using
> > the DPAD (RGB) output, the DU driver thus enables and disables the LVDS
> > PLL manually, while when using the LVDS output, it lets the LVDS bridge
> > driver handle the PLL configuration internally as part of the atomic
> > enable and disable operations.
> >
> > This causes an issue when using the LVDS output. As bridges are disabled
> > before CRTCs, the current implementation violates the enable/disable
> > sequences documented in the hardware datasheet, which requires the dot
> > clock to be enabled before the CRTC is started and disabled after it
> > gets stopped.
> >
> > Fix the problem by enabling/disabling the LVDS PLL manually from the DU
> > regardless of which output is used, and skipping the PLL handling in the
> > LVDS bridge atomic enable and disable operations.
> >
> > This is however not enough. Disabling the LVDS encoder while leaving the
> > PLL on still results in a vertical blanking wait timeout when disabling
> > the DU. Investigation showed that the culprit is the LVEN bit. For an
> > unclear reason, clearing the bit when disabling the LVDS encoder blocks
> > vertical blanking interrupts. We thus have to delay disabling the whole
> > LVDS encoder, not just disabling the PLL, until the DU is disabled.
> >
> > We could split the LVDS disable sequence by clearing the LVRES bit in
> > the LVDS bridge atomic disable handler, and delaying the rest of the
> > operations, in order to disable the LVDS output at bridge atomic disable
> > time, before stopping the CRTC. This would make the code more complex,
> > without a clear benefit, so keep the implementation simple(r).
>
> The asymmetry of all this makes me grit my teeth, but SW has to do what
> SW has to do...
I have the same feeling :-( It would be nice if the DU and LVDS were
better partitioned. And if we knew exactly what the hardware
requirements are regarding the start and stop sequences. Maybe we're
spoiled, we're not happy when we can get datasheets, and dream of
getting the RTL. Of course it may well turn into a nightmare if we
actually had access to that.
> Just a minor comment below, other than that:
>
> Reviewed-by: Tomi Valkeinen
>
> > Signed-off-by: Laurent Pinchart
> > ---
> > drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 18 ++--
> > drivers/gpu/drm/rcar-du/rcar_lvds.c| 114 +++--
> > drivers/gpu/drm/rcar-du/rcar_lvds.h| 12 ++-
> > 3 files changed, 86 insertions(+), 58 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> > b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> > index 008e172ed43b..71e7fbace38d 100644
> > --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> > +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> > @@ -749,16 +749,17 @@ static void rcar_du_crtc_atomic_enable(struct
> > drm_crtc *crtc,
> >
> > /*
> > * 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
> > -* the LVDS output is disabled.
> > +* the DU channel. We need to enable its clock output explicitly before
> > +* starting the CRTC, as the bridge hasn't been enabled by the atomic
> > +* helpers yet.
> > */
> > - if (rcdu->info->lvds_clk_mask & BIT(rcrtc->index) &&
> > - rstate->outputs == BIT(RCAR_DU_OUTPUT_DPAD0)) {
> > + if (rcdu->info->lvds_clk_mask & BIT(rcrtc->index)) {
> > + bool dot_clk_only = rstate->outputs ==
> > BIT(RCAR_DU_OUTPUT_DPAD0);
> > struct drm_bridge *bridge = rcdu->lvds[rcrtc->index];
> > const struct drm_display_mode *mode =
> > >state->adjusted_mode;
> >
> > - rcar_lvds_pclk_enable(bridge, mode->clock * 1000);
> > + rcar_lvds_pclk_enable(bridge, mode->clock * 1000, dot_clk_only);
> > }
> >
> > /*
> > @@ -795,15 +796,15 @@ static void rcar_du_crtc_atomic_disable(struct
> > drm_crtc *crtc,
> > rcar_du_crtc_stop(rcrtc);
> > rcar_du_crtc_put(rcrtc);
> >
> > - if (rcdu->info->lvds_clk_mask & BIT(rcrtc->index) &&
> > - rstate->outputs == BIT(RCAR_DU_OUTPUT_DPAD0)) {
> > + if (rcdu->info->lvds_clk_mask & BIT(rcrtc->index)) {
> > + bool dot_clk_only = rstate->outputs ==
> > BIT(RCAR_DU_OUTPUT_DPAD0);
> > struct drm_bridge *bridge = rcdu->lvds[rcrtc->index];
> >
> > /*
> > * Disable the LVDS clock output, see
> > * rcar_du_crtc_atomic_enable().
> > */
>
> This could mention that when LVDS output is used, also the LVDS output
> is disabled here.
I'll write
/*
* Disable the LVDS clock output, see
* rcar_du_crtc_atomic_enable(). When the LVDS
Re: [PATCH 3/3] drm: rcar-du: lvds: Fix LVDS PLL disable on D3/E3
On 14/02/2023 02:37, Laurent Pinchart wrote:
On R-Car D3 and E3, the LVDS encoder provides the dot (pixel) clock to
the DU, regardless of whether the LVDS output is used or not. When using
the DPAD (RGB) output, the DU driver thus enables and disables the LVDS
PLL manually, while when using the LVDS output, it lets the LVDS bridge
driver handle the PLL configuration internally as part of the atomic
enable and disable operations.
This causes an issue when using the LVDS output. As bridges are disabled
before CRTCs, the current implementation violates the enable/disable
sequences documented in the hardware datasheet, which requires the dot
clock to be enabled before the CRTC is started and disabled after it
gets stopped.
Fix the problem by enabling/disabling the LVDS PLL manually from the DU
regardless of which output is used, and skipping the PLL handling in the
LVDS bridge atomic enable and disable operations.
This is however not enough. Disabling the LVDS encoder while leaving the
PLL on still results in a vertical blanking wait timeout when disabling
the DU. Investigation showed that the culprit is the LVEN bit. For an
unclear reason, clearing the bit when disabling the LVDS encoder blocks
vertical blanking interrupts. We thus have to delay disabling the whole
LVDS encoder, not just disabling the PLL, until the DU is disabled.
We could split the LVDS disable sequence by clearing the LVRES bit in
the LVDS bridge atomic disable handler, and delaying the rest of the
operations, in order to disable the LVDS output at bridge atomic disable
time, before stopping the CRTC. This would make the code more complex,
without a clear benefit, so keep the implementation simple(r).
The asymmetry of all this makes me grit my teeth, but SW has to do what
SW has to do... Just a minor comment below, other than that:
Reviewed-by: Tomi Valkeinen
Signed-off-by: Laurent Pinchart
---
drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 18 ++--
drivers/gpu/drm/rcar-du/rcar_lvds.c| 114 +++--
drivers/gpu/drm/rcar-du/rcar_lvds.h| 12 ++-
3 files changed, 86 insertions(+), 58 deletions(-)
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
index 008e172ed43b..71e7fbace38d 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
@@ -749,16 +749,17 @@ static void rcar_du_crtc_atomic_enable(struct drm_crtc
*crtc,
/*
* 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
-* the LVDS output is disabled.
+* the DU channel. We need to enable its clock output explicitly before
+* starting the CRTC, as the bridge hasn't been enabled by the atomic
+* helpers yet.
*/
- if (rcdu->info->lvds_clk_mask & BIT(rcrtc->index) &&
- rstate->outputs == BIT(RCAR_DU_OUTPUT_DPAD0)) {
+ if (rcdu->info->lvds_clk_mask & BIT(rcrtc->index)) {
+ bool dot_clk_only = rstate->outputs ==
BIT(RCAR_DU_OUTPUT_DPAD0);
struct drm_bridge *bridge = rcdu->lvds[rcrtc->index];
const struct drm_display_mode *mode =
>state->adjusted_mode;
- rcar_lvds_pclk_enable(bridge, mode->clock * 1000);
+ rcar_lvds_pclk_enable(bridge, mode->clock * 1000, dot_clk_only);
}
/*
@@ -795,15 +796,15 @@ static void rcar_du_crtc_atomic_disable(struct drm_crtc
*crtc,
rcar_du_crtc_stop(rcrtc);
rcar_du_crtc_put(rcrtc);
- if (rcdu->info->lvds_clk_mask & BIT(rcrtc->index) &&
- rstate->outputs == BIT(RCAR_DU_OUTPUT_DPAD0)) {
+ if (rcdu->info->lvds_clk_mask & BIT(rcrtc->index)) {
+ bool dot_clk_only = rstate->outputs ==
BIT(RCAR_DU_OUTPUT_DPAD0);
struct drm_bridge *bridge = rcdu->lvds[rcrtc->index];
/*
* Disable the LVDS clock output, see
* rcar_du_crtc_atomic_enable().
*/
This could mention that when LVDS output is used, also the LVDS output
is disabled here.
- rcar_lvds_pclk_disable(bridge);
+ rcar_lvds_pclk_disable(bridge, dot_clk_only);
}
if ((rcdu->info->dsi_clk_mask & BIT(rcrtc->index)) &&
@@ -815,7 +816,6 @@ static void rcar_du_crtc_atomic_disable(struct drm_crtc
*crtc,
* Disable the DSI clock output, see
* rcar_du_crtc_atomic_enable().
*/
-
rcar_mipi_dsi_pclk_disable(bridge);
}
diff --git a/drivers/gpu/drm/rcar-du/rcar_lvds.c b/drivers/gpu/drm/rcar-du/rcar_lvds.c
index 70cdd5ec64d5..ca215b588fd7 100644
--- a/drivers/gpu/drm/rcar-du/rcar_lvds.c
+++ b/drivers/gpu/drm/rcar-du/rcar_lvds.c
@@ -269,8 +269,8 @@ static void rcar_lvds_d3_e3_pll_calc(struct rcar_lvds
*lvds, struct clk *clk,
pll->pll_m, pll->pll_n, pll->pll_e, pll->div);
}
