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


Nobuhiro Iwamatsu
 

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(岩松 信洋 □SWC◯ACT)
<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?
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.

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


Biju Das <biju.das.jz@...>
 

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(岩松 信洋 □SWC◯
ACT)
<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


Pavel Machek
 

Hi!

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.
The patch looks okay to me. I'll test it and can apply it if it works
okay.

But I don't believe it is in our 5.10, so I'd like it to be submitted
for 5.10, too.

Or just tell me it is okay and I'll apply it there, too.

Best regards,
Pavel

--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany


Pavel Machek
 

Hi!

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.
The patch looks okay to me. I'll test it and can apply it if it works
okay.

But I don't believe it is in our 5.10, so I'd like it to be submitted for
5.10, too.
Applied and pushed out.

Or just tell me it is okay and I'll apply it there, too.
Please apply to 5.10-cip as well, since it is present in 5.10 stable.
Aha, ok, that changs things. (I checked our trees but not stable).

In such case, nothing needs to be done at the moment, and it is easier
to wait till we merge that through the stable tree.

Best regards,
Pavel

--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany


Biju Das <biju.das.jz@...>
 

Hi Pavel,

Thanks for the feedback.

-----Original Message-----
From: Pavel Machek <pavel@denx.de>
Sent: 13 April 2021 11:12
To: Biju Das <biju.das.jz@bp.renesas.com>
Cc: cip-dev@lists.cip-project.org; Nobuhiro Iwamatsu
<nobuhiro1.iwamatsu@toshiba.co.jp>; Pavel Machek <pavel@denx.de>; Chris
Paterson <Chris.Paterson2@renesas.com>; Prabhakar Mahadev Lad
<prabhakar.mahadev-lad.rj@bp.renesas.com>
Subject: Re: [PATCH 4.19.y-cip] drm: rcar-du: Fix crash when using LVDS1
clock for CRTC

Hi!

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.
The patch looks okay to me. I'll test it and can apply it if it works
okay.

But I don't believe it is in our 5.10, so I'd like it to be submitted for
5.10, too.

Or just tell me it is okay and I'll apply it there, too.
Please apply to 5.10-cip as well, since it is present in 5.10 stable.

Cheers,
Biju


Biju Das <biju.das.jz@...>
 

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

Hi!

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.
The patch looks okay to me. I'll test it and can apply it if it
works okay.

But I don't believe it is in our 5.10, so I'd like it to be
submitted for 5.10, too.
Applied and pushed out.
Thanks you.

Regards,
Biju