Re: [cip-dev] [PATCH 4.19.y-cip] drm: rcar-du: Fix crash when using LVDS1 clock for CRTC

[Biju Das <biju.das.jz@bp.renesas.com>]

[-- Attachment #1: Type: text/plain, Size: 6566 bytes --]

Hi Nobuhiro,

Thanks for the feedback.

> Subject: RE: [PATCH 4.19.y-cip] drm: rcar-du: Fix crash when using LVDS1
> clock for CRTC
> 
> Hi,
> 
> > -----Original Message-----
> > From: Biju Das [mailto:biju.das.jz@bp.renesas.com]
> > Sent: Saturday, April 10, 2021 1:15 AM
> > To: cip-dev@lists.cip-project.org; iwamatsu nobuhiro(岩松 信洋 □ＳＷＣ◯
> ＡＣＴ)
> > <nobuhiro1.iwamatsu@toshiba.co.jp>; Pavel Machek <pavel@denx.de>
> > Cc: Chris Paterson <chris.paterson2@renesas.com>; Biju Das
> > <biju.das.jz@bp.renesas.com>; Prabhakar Mahadev Lad
> > <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > Subject: [PATCH 4.19.y-cip] drm: rcar-du: Fix crash when using LVDS1
> > clock for CRTC
> >
> > From: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> >
> > [ Upstream commit 53ced169373aab52d3b5da0fee6a342002d1876d ]
> >
> > 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. The rcar_du_device encoders field isn't used
> > anymore and can be dropped.
> >
> > Fixes: 8e8fddab0d0a ("drm: rcar-du: Skip LVDS1 output on Gen3 when
> > using dual-link LVDS mode")
> > Signed-off-by: Laurent Pinchart
> > <laurent.pinchart+renesas@ideasonboard.com>
> > Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> > Signed-off-by: Sasha Levin <sashal@kernel.org>
> 
> Commit: 53ced169373aab52d3b5da0fee6a342002d1876d does not have Sasha's
> singed-off.
> # Backported from 5.11 or 5.10?

Yes, I backported this patch from 5.10 kernel.

> If you backported from 53ced1693, I don't think this is necessary.
> > [Biju: Manually applied the changes to adapt with 4.19 kernel]
> > Signed-off-by: Biju Das <biju.das@bp.renesas.com>
> 
> It may be a problem in my environment, I can not apply this patch to
> linux-4.19.y-cip tree.
> And I can not find this mail from cip-dev ML and patchwork.

I also noticed this patch is not there in cip-dev ML or in patchwork.

Cheers,
Biju


> Best regards,
>   Nobuhiro
> 
> > ---
> >  drivers/gpu/drm/rcar-du/rcar_du_crtc.c    | 11 ++++-------
> >  drivers/gpu/drm/rcar-du/rcar_du_drv.h     |  6 +++---
> >  drivers/gpu/drm/rcar-du/rcar_du_encoder.c |  5 ++++-
> >  3 files changed, 11 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> > b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> > index cec4b5c21eea..3362ccced05a 100644
> > --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> > +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> > @@ -676,13 +676,11 @@ 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;
> >
> > -		rcar_lvds_clk_enable(encoder->base.bridge,
> > -				     mode->clock * 1000);
> > +		rcar_lvds_clk_enable(bridge, mode->clock * 1000);
> >  	}
> >
> >  	rcar_du_crtc_start(rcrtc);
> > @@ -700,14 +698,13 @@ 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 = rcdu->lvds[rcrtc->index];
> >
> >  		/*
> >  		 * Disable the LVDS clock output, see
> >  		 * rcar_du_crtc_atomic_enable().
> >  		 */
> > -		rcar_lvds_clk_disable(encoder->base.bridge);
> > +		rcar_lvds_clk_disable(bridge);
> >  	}
> >
> >  	spin_lock_irq(&crtc->dev->event_lock);
> > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.h
> > b/drivers/gpu/drm/rcar-du/rcar_du_drv.h
> > index f16cd263d2c1..558ebefce4ac 100644
> > --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.h
> > +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.h
> > @@ -23,9 +23,9 @@
> >
> >  struct clk;
> >  struct device;
> > +struct drm_bridge;
> >  struct drm_device;
> >  struct rcar_du_device;
> > -struct rcar_du_encoder;
> >
> >  #define RCAR_DU_FEATURE_CRTC_IRQ_CLOCK	BIT(0)	/* Per-CRTC IRQ and
> clock */
> >  #define RCAR_DU_FEATURE_VSP1_SOURCE	BIT(1)	/* Has inputs from
> VSP1 */
> > @@ -73,6 +73,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;
> > @@ -87,10 +88,9 @@ struct rcar_du_device {
> >  	struct rcar_du_crtc crtcs[RCAR_DU_MAX_CRTCS];
> >  	unsigned int num_crtcs;
> >
> > -	struct rcar_du_encoder *encoders[RCAR_DU_OUTPUT_MAX];
> > -
> >  	struct rcar_du_group groups[RCAR_DU_MAX_GROUPS];
> >  	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 de8fe74c0362..325e301f97e3 100644
> > --- a/drivers/gpu/drm/rcar-du/rcar_du_encoder.c
> > +++ b/drivers/gpu/drm/rcar-du/rcar_du_encoder.c
> > @@ -47,7 +47,6 @@ int rcar_du_encoder_init(struct rcar_du_device *rcdu,
> >  	if (renc == NULL)
> >  		return -ENOMEM;
> >
> > -	rcdu->encoders[output] = renc;
> >  	renc->output = output;
> >  	encoder = rcar_encoder_to_drm_encoder(renc);
> >
> > @@ -61,6 +60,10 @@ int rcar_du_encoder_init(struct rcar_du_device *rcdu,
> >  		goto done;
> >  	}
> >
> > +	if (output == RCAR_DU_OUTPUT_LVDS0 ||
> > +	    output == RCAR_DU_OUTPUT_LVDS1)
> > +		rcdu->lvds[output - RCAR_DU_OUTPUT_LVDS0] = bridge;
> > +
> >  	/*
> >  	 * On Gen3 skip the LVDS1 output if the LVDS1 encoder is used as a
> >  	 * companion for LVDS0 in dual-link mode.
> > --
> > 2.17.1


[-- Attachment #2: Type: text/plain, Size: 428 bytes --]


-=-=-=-=-=-=-=-=-=-=-=-
Links: You receive all messages sent to this group.
View/Reply Online (#6358): https://lists.cip-project.org/g/cip-dev/message/6358
Mute This Topic: https://lists.cip-project.org/mt/82032629/4520388
Group Owner: cip-dev+owner@lists.cip-project.org
Unsubscribe: https://lists.cip-project.org/g/cip-dev/leave/8129055/4520388/727948398/xyzzy [cip-dev@archiver.kernel.org]
-=-=-=-=-=-=-=-=-=-=-=-