From: Jacopo Mondi <jacopo@jmondi.org> To: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> Cc: dri-devel@lists.freedesktop.org, linux-renesas-soc@vger.kernel.org, Kieran Bingham <kieran.bingham@ideasonboard.com>, Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> Subject: Re: [PATCH 1/9] drm: rcar-du: Fix crash when using LVDS1 clock for CRTC Date: Mon, 14 Dec 2020 10:43:20 +0100 [thread overview] Message-ID: <20201214094320.6hsku3e2ydvlkgqb@uno.localdomain> (raw) In-Reply-To: <20201204220139.15272-2-laurent.pinchart+renesas@ideasonboard.com> 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
WARNING: multiple messages have this Message-ID (diff)
From: Jacopo Mondi <jacopo@jmondi.org> To: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> Cc: linux-renesas-soc@vger.kernel.org, Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>, Kieran Bingham <kieran.bingham@ideasonboard.com>, dri-devel@lists.freedesktop.org Subject: Re: [PATCH 1/9] drm: rcar-du: Fix crash when using LVDS1 clock for CRTC Date: Mon, 14 Dec 2020 10:43:20 +0100 [thread overview] Message-ID: <20201214094320.6hsku3e2ydvlkgqb@uno.localdomain> (raw) In-Reply-To: <20201204220139.15272-2-laurent.pinchart+renesas@ideasonboard.com> 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 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
next prev parent reply other threads:[~2020-12-14 9:43 UTC|newest] Thread overview: 66+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-12-04 22:01 [PATCH 0/9] drm: rcar-du: Fix LVDS-related crash Laurent Pinchart 2020-12-04 22:01 ` Laurent Pinchart 2020-12-04 22:01 ` [PATCH 1/9] drm: rcar-du: Fix crash when using LVDS1 clock for CRTC Laurent Pinchart 2020-12-04 22:01 ` Laurent Pinchart 2020-12-07 8:15 ` Geert Uytterhoeven 2020-12-07 8:15 ` Geert Uytterhoeven 2020-12-08 0:42 ` Laurent Pinchart 2020-12-08 0:42 ` Laurent Pinchart 2020-12-14 9:43 ` Jacopo Mondi [this message] 2020-12-14 9:43 ` Jacopo Mondi 2020-12-14 15:36 ` Kieran Bingham 2020-12-14 15:36 ` Kieran Bingham 2020-12-04 22:01 ` [PATCH 2/9] drm: rcar-du: Release vsp device reference in all error paths Laurent Pinchart 2020-12-04 22:01 ` Laurent Pinchart 2020-12-14 10:03 ` Jacopo Mondi 2020-12-14 10:03 ` Jacopo Mondi 2020-12-14 15:42 ` Kieran Bingham 2020-12-14 15:42 ` Kieran Bingham 2020-12-04 22:01 ` [PATCH 3/9] drm: rcar-du: Drop unneeded encoder cleanup in error path Laurent Pinchart 2020-12-04 22:01 ` Laurent Pinchart 2020-12-14 10:11 ` Jacopo Mondi 2020-12-14 10:11 ` Jacopo Mondi 2020-12-14 13:19 ` Laurent Pinchart 2020-12-14 13:19 ` Laurent Pinchart 2020-12-14 15:59 ` Kieran Bingham 2020-12-14 15:59 ` Kieran Bingham 2020-12-04 22:01 ` [PATCH 4/9] drm: rcar-du: Use DRM-managed allocation for VSP planes Laurent Pinchart 2020-12-04 22:01 ` Laurent Pinchart 2020-12-14 16:20 ` Kieran Bingham 2020-12-14 16:20 ` Kieran Bingham 2020-12-14 16:26 ` Laurent Pinchart 2020-12-14 16:26 ` Laurent Pinchart 2020-12-15 15:42 ` Geert Uytterhoeven 2020-12-15 15:42 ` Geert Uytterhoeven 2020-12-04 22:01 ` [PATCH 5/9] drm: rcar-du: Use DRM-managed allocation for encoders Laurent Pinchart 2020-12-04 22:01 ` Laurent Pinchart 2020-12-14 10:37 ` Jacopo Mondi 2020-12-14 10:37 ` Jacopo Mondi 2020-12-14 13:28 ` Laurent Pinchart 2020-12-14 13:28 ` Laurent Pinchart 2020-12-04 22:01 ` [PATCH 6/9] drm: rcar-du: Embed drm_device in rcar_du_device Laurent Pinchart 2020-12-04 22:01 ` Laurent Pinchart 2020-12-14 10:55 ` Jacopo Mondi 2020-12-14 10:55 ` Jacopo Mondi 2020-12-14 17:02 ` Kieran Bingham 2020-12-14 17:02 ` Kieran Bingham 2020-12-04 22:01 ` [PATCH 7/9] drm: rcar-du: Replace dev_private with container_of Laurent Pinchart 2020-12-04 22:01 ` Laurent Pinchart 2020-12-14 10:58 ` Jacopo Mondi 2020-12-14 10:58 ` Jacopo Mondi 2020-12-14 17:11 ` Kieran Bingham 2020-12-14 17:11 ` Kieran Bingham 2020-12-04 22:01 ` [PATCH 8/9] drm: rcar-du: Skip encoder allocation for LVDS1 in dual-link mode Laurent Pinchart 2020-12-04 22:01 ` Laurent Pinchart 2020-12-14 11:04 ` Jacopo Mondi 2020-12-14 11:04 ` Jacopo Mondi 2020-12-14 13:32 ` Laurent Pinchart 2020-12-14 13:32 ` Laurent Pinchart 2020-12-14 17:21 ` Kieran Bingham 2020-12-14 17:21 ` Kieran Bingham 2020-12-04 22:01 ` [PATCH 9/9] drm: rcar-du: Drop local encoder variable Laurent Pinchart 2020-12-04 22:01 ` Laurent Pinchart 2020-12-14 11:05 ` Jacopo Mondi 2020-12-14 11:05 ` Jacopo Mondi 2020-12-14 17:16 ` Kieran Bingham 2020-12-14 17:16 ` Kieran Bingham
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20201214094320.6hsku3e2ydvlkgqb@uno.localdomain \ --to=jacopo@jmondi.org \ --cc=dri-devel@lists.freedesktop.org \ --cc=kieran.bingham@ideasonboard.com \ --cc=kuninori.morimoto.gx@renesas.com \ --cc=laurent.pinchart+renesas@ideasonboard.com \ --cc=linux-renesas-soc@vger.kernel.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.