| Message ID | 20201204220139.15272-2-laurent.pinchart+renesas@ideasonboard.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Kieran Bingham |
| Headers | show |
| Series | drm: rcar-du: Fix LVDS-related crash | expand |
Hi Laurent, On Fri, Dec 4, 2020 at 11:02 PM Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> wrote: > On D3 and E3 platforms, the LVDS encoder includes a PLL that can > generate a clock for the corresponding CRTC, used even when the CRTC > output to a non-LVDS port. This mechanism is supported by the driver, > but the implementation is broken in dual-link LVDS mode. In that case, > the LVDS1 drm_encoder is skipped, which causes a crash when trying to > access its bridge later on. > > Fix this by storing bridge pointers internally instead of retrieving > them from the encoder. > > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> Thanks for your patch! I think this warrants a Fixes tag, to assist the stable team in backporting this fix. Gr{oetje,eeting}s, Geert
Geert, On Mon, Dec 07, 2020 at 09:15:11AM +0100, Geert Uytterhoeven wrote: > On Fri, Dec 4, 2020 at 11:02 PM Laurent Pinchart wrote: > > On D3 and E3 platforms, the LVDS encoder includes a PLL that can > > generate a clock for the corresponding CRTC, used even when the CRTC > > output to a non-LVDS port. This mechanism is supported by the driver, > > but the implementation is broken in dual-link LVDS mode. In that case, > > the LVDS1 drm_encoder is skipped, which causes a crash when trying to > > access its bridge later on. > > > > Fix this by storing bridge pointers internally instead of retrieving > > them from the encoder. > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> > > Thanks for your patch! > > I think this warrants a Fixes tag, to assist the stable team in backporting > this fix. I'll add one.
Hi Laurent, On Sat, Dec 05, 2020 at 12:01:31AM +0200, Laurent Pinchart wrote: > On D3 and E3 platforms, the LVDS encoder includes a PLL that can > generate a clock for the corresponding CRTC, used even when the CRTC > output to a non-LVDS port. This mechanism is supported by the driver, > but the implementation is broken in dual-link LVDS mode. In that case, > the LVDS1 drm_encoder is skipped, which causes a crash when trying to > access its bridge later on. Looking at a dts example with both lvds encoders and RGB output enabled, I might have missed why the LVDS1 encoder is skipped. > > Fix this by storing bridge pointers internally instead of retrieving > them from the encoder. > > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> The patch itself looks good! Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org> Thanks j > --- > drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 10 ++-------- > drivers/gpu/drm/rcar-du/rcar_du_drv.h | 3 +++ > drivers/gpu/drm/rcar-du/rcar_du_encoder.c | 4 ++++ > 3 files changed, 9 insertions(+), 8 deletions(-) > > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c > index b5fb941e0f53..e23b9c7b4afe 100644 > --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c > +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c > @@ -730,13 +730,10 @@ static void rcar_du_crtc_atomic_enable(struct drm_crtc *crtc, > */ > if (rcdu->info->lvds_clk_mask & BIT(rcrtc->index) && > rstate->outputs == BIT(RCAR_DU_OUTPUT_DPAD0)) { > - struct rcar_du_encoder *encoder = > - rcdu->encoders[RCAR_DU_OUTPUT_LVDS0 + rcrtc->index]; > + struct drm_bridge *bridge = rcdu->lvds[rcrtc->index]; > const struct drm_display_mode *mode = > &crtc->state->adjusted_mode; > - struct drm_bridge *bridge; > > - bridge = drm_bridge_chain_get_first_bridge(&encoder->base); > rcar_lvds_clk_enable(bridge, mode->clock * 1000); > } > > @@ -764,15 +761,12 @@ static void rcar_du_crtc_atomic_disable(struct drm_crtc *crtc, > > if (rcdu->info->lvds_clk_mask & BIT(rcrtc->index) && > rstate->outputs == BIT(RCAR_DU_OUTPUT_DPAD0)) { > - struct rcar_du_encoder *encoder = > - rcdu->encoders[RCAR_DU_OUTPUT_LVDS0 + rcrtc->index]; > - struct drm_bridge *bridge; > + struct drm_bridge *bridge = rcdu->lvds[rcrtc->index]; > > /* > * Disable the LVDS clock output, see > * rcar_du_crtc_atomic_enable(). > */ > - bridge = drm_bridge_chain_get_first_bridge(&encoder->base); > rcar_lvds_clk_disable(bridge); > } > > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.h b/drivers/gpu/drm/rcar-du/rcar_du_drv.h > index 61504c54e2ec..71732fc5df8f 100644 > --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.h > +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.h > @@ -20,6 +20,7 @@ > > struct clk; > struct device; > +struct drm_bridge; > struct drm_device; > struct drm_property; > struct rcar_du_device; > @@ -71,6 +72,7 @@ struct rcar_du_device_info { > #define RCAR_DU_MAX_CRTCS 4 > #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 > > struct rcar_du_device { > struct device *dev; > @@ -88,6 +90,7 @@ struct rcar_du_device { > struct rcar_du_group groups[RCAR_DU_MAX_GROUPS]; > 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 { > 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 b0335da0c161..2d40da98144b 100644 > --- a/drivers/gpu/drm/rcar-du/rcar_du_encoder.c > +++ b/drivers/gpu/drm/rcar-du/rcar_du_encoder.c > @@ -91,6 +91,10 @@ int rcar_du_encoder_init(struct rcar_du_device *rcdu, > ret = -EPROBE_DEFER; > goto done; > } > + > + if (output == RCAR_DU_OUTPUT_LVDS0 || > + output == RCAR_DU_OUTPUT_LVDS1) > + rcdu->lvds[output - RCAR_DU_OUTPUT_LVDS0] = bridge; > } > > /* > -- > Regards, > > Laurent Pinchart > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
Hi Laurent, On 04/12/2020 22:01, Laurent Pinchart wrote: > On D3 and E3 platforms, the LVDS encoder includes a PLL that can > generate a clock for the corresponding CRTC, used even when the CRTC > output to a non-LVDS port. This mechanism is supported by the driver, > but the implementation is broken in dual-link LVDS mode. In that case, > the LVDS1 drm_encoder is skipped, which causes a crash when trying to > access its bridge later on. > > Fix this by storing bridge pointers internally instead of retrieving > them from the encoder. > This looks cleaner too IMO. Win win, bug fix and nicer code. Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com> > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> > --- > drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 10 ++-------- > drivers/gpu/drm/rcar-du/rcar_du_drv.h | 3 +++ > drivers/gpu/drm/rcar-du/rcar_du_encoder.c | 4 ++++ > 3 files changed, 9 insertions(+), 8 deletions(-) > > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c > index b5fb941e0f53..e23b9c7b4afe 100644 > --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c > +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c > @@ -730,13 +730,10 @@ static void rcar_du_crtc_atomic_enable(struct drm_crtc *crtc, > */ > if (rcdu->info->lvds_clk_mask & BIT(rcrtc->index) && > rstate->outputs == BIT(RCAR_DU_OUTPUT_DPAD0)) { > - struct rcar_du_encoder *encoder = > - rcdu->encoders[RCAR_DU_OUTPUT_LVDS0 + rcrtc->index]; > + struct drm_bridge *bridge = rcdu->lvds[rcrtc->index]; > const struct drm_display_mode *mode = > &crtc->state->adjusted_mode; > - struct drm_bridge *bridge; > > - bridge = drm_bridge_chain_get_first_bridge(&encoder->base); > rcar_lvds_clk_enable(bridge, mode->clock * 1000); > } > > @@ -764,15 +761,12 @@ static void rcar_du_crtc_atomic_disable(struct drm_crtc *crtc, > > if (rcdu->info->lvds_clk_mask & BIT(rcrtc->index) && > rstate->outputs == BIT(RCAR_DU_OUTPUT_DPAD0)) { > - struct rcar_du_encoder *encoder = > - rcdu->encoders[RCAR_DU_OUTPUT_LVDS0 + rcrtc->index]; > - struct drm_bridge *bridge; > + struct drm_bridge *bridge = rcdu->lvds[rcrtc->index]; > > /* > * Disable the LVDS clock output, see > * rcar_du_crtc_atomic_enable(). > */ > - bridge = drm_bridge_chain_get_first_bridge(&encoder->base); > rcar_lvds_clk_disable(bridge); > } > > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.h b/drivers/gpu/drm/rcar-du/rcar_du_drv.h > index 61504c54e2ec..71732fc5df8f 100644 > --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.h > +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.h > @@ -20,6 +20,7 @@ > > struct clk; > struct device; > +struct drm_bridge; > struct drm_device; > struct drm_property; > struct rcar_du_device; > @@ -71,6 +72,7 @@ struct rcar_du_device_info { > #define RCAR_DU_MAX_CRTCS 4 > #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 > > struct rcar_du_device { > struct device *dev; > @@ -88,6 +90,7 @@ struct rcar_du_device { > struct rcar_du_group groups[RCAR_DU_MAX_GROUPS]; > 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 { > 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 b0335da0c161..2d40da98144b 100644 > --- a/drivers/gpu/drm/rcar-du/rcar_du_encoder.c > +++ b/drivers/gpu/drm/rcar-du/rcar_du_encoder.c > @@ -91,6 +91,10 @@ int rcar_du_encoder_init(struct rcar_du_device *rcdu, > ret = -EPROBE_DEFER; > goto done; > } > + > + if (output == RCAR_DU_OUTPUT_LVDS0 || > + output == RCAR_DU_OUTPUT_LVDS1) > + rcdu->lvds[output - RCAR_DU_OUTPUT_LVDS0] = bridge; > } > > /* >
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c index b5fb941e0f53..e23b9c7b4afe 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c @@ -730,13 +730,10 @@ static void rcar_du_crtc_atomic_enable(struct drm_crtc *crtc, */ if (rcdu->info->lvds_clk_mask & BIT(rcrtc->index) && rstate->outputs == BIT(RCAR_DU_OUTPUT_DPAD0)) { - struct rcar_du_encoder *encoder = - rcdu->encoders[RCAR_DU_OUTPUT_LVDS0 + rcrtc->index]; + struct drm_bridge *bridge = rcdu->lvds[rcrtc->index]; const struct drm_display_mode *mode = &crtc->state->adjusted_mode; - struct drm_bridge *bridge; - bridge = drm_bridge_chain_get_first_bridge(&encoder->base); rcar_lvds_clk_enable(bridge, mode->clock * 1000); } @@ -764,15 +761,12 @@ static void rcar_du_crtc_atomic_disable(struct drm_crtc *crtc, if (rcdu->info->lvds_clk_mask & BIT(rcrtc->index) && rstate->outputs == BIT(RCAR_DU_OUTPUT_DPAD0)) { - struct rcar_du_encoder *encoder = - rcdu->encoders[RCAR_DU_OUTPUT_LVDS0 + rcrtc->index]; - struct drm_bridge *bridge; + struct drm_bridge *bridge = rcdu->lvds[rcrtc->index]; /* * Disable the LVDS clock output, see * rcar_du_crtc_atomic_enable(). */ - bridge = drm_bridge_chain_get_first_bridge(&encoder->base); rcar_lvds_clk_disable(bridge); } diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.h b/drivers/gpu/drm/rcar-du/rcar_du_drv.h index 61504c54e2ec..71732fc5df8f 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.h +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.h @@ -20,6 +20,7 @@ struct clk; struct device; +struct drm_bridge; struct drm_device; struct drm_property; struct rcar_du_device; @@ -71,6 +72,7 @@ struct rcar_du_device_info { #define RCAR_DU_MAX_CRTCS 4 #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 struct rcar_du_device { struct device *dev; @@ -88,6 +90,7 @@ struct rcar_du_device { struct rcar_du_group groups[RCAR_DU_MAX_GROUPS]; 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 { 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 b0335da0c161..2d40da98144b 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_encoder.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_encoder.c @@ -91,6 +91,10 @@ int rcar_du_encoder_init(struct rcar_du_device *rcdu, ret = -EPROBE_DEFER; goto done; } + + if (output == RCAR_DU_OUTPUT_LVDS0 || + output == RCAR_DU_OUTPUT_LVDS1) + rcdu->lvds[output - RCAR_DU_OUTPUT_LVDS0] = bridge; } /*
On D3 and E3 platforms, the LVDS encoder includes a PLL that can generate a clock for the corresponding CRTC, used even when the CRTC output to a non-LVDS port. This mechanism is supported by the driver, but the implementation is broken in dual-link LVDS mode. In that case, the LVDS1 drm_encoder is skipped, which causes a crash when trying to access its bridge later on. Fix this by storing bridge pointers internally instead of retrieving them from the encoder. Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> --- drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 10 ++-------- drivers/gpu/drm/rcar-du/rcar_du_drv.h | 3 +++ drivers/gpu/drm/rcar-du/rcar_du_encoder.c | 4 ++++ 3 files changed, 9 insertions(+), 8 deletions(-)