drm: rcar-du: Fix R-Car Gen3 crash when VSP is disabled

PATCH In review Magnus Damm 3 participants 6 messages Oct 26, 2016 – Dec 14, 2016

Subsystems (3)

DRM drivers DRM misc Arm/RISC-V/Renesas architecture

lore

Share & tools

lore

Magnus Damm

author maintainer

2016-10-26 03:31 UTC #0 lore raw

PATCH 1/1 drm: rcar-du: Fix R-Car Gen3 crash when VSP is disabled

From: Magnus Damm <[email protected]>

For the DU to operate on R-Car Gen3 hardware a combination of DU and VSP devices are required. Since the DU driver also supports earlier generations hardware the VSP portion is enabled via Kconfig.

The arm64 defconfig is as of v4.9-rc1 having the DU driver enabled as a module, however this is not enough to support R-Car Gen3. In the current case of CONFIG_DRM_RCAR_VSP=n then the kernel crashes when loading the module. This patch is fixing that particular case.

In more detail, the crash triggers in drm_atomic_get_plane_state() when __drm_atomic_helper_set_config() passes NULL as crtc->primary.

This patch corrects this issue by failing to load the DU driver on R-Car Gen3 when VSP is not available.

Signed-off-by Magnus Damm <[email protected]>

---

 drivers/gpu/drm/rcar-du/rcar_du_vsp.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/gpu/drm/rcar-du/rcar_du_vsp.h +1 -1

@@ -70,7 +70,7 @@ void rcar_du_vsp_disable(struct rcar_du_
 void rcar_du_vsp_atomic_begin(struct rcar_du_crtc *crtc);
 void rcar_du_vsp_atomic_flush(struct rcar_du_crtc *crtc);
 #else
-static inline int rcar_du_vsp_init(struct rcar_du_vsp *vsp) { return 0; };
+static inline int rcar_du_vsp_init(struct rcar_du_vsp *vsp) { return -ENXIO; };
 static inline void rcar_du_vsp_enable(struct rcar_du_crtc *crtc) { };
 static inline void rcar_du_vsp_disable(struct rcar_du_crtc *crtc) { };
 static inline void rcar_du_vsp_atomic_begin(struct rcar_du_crtc *crtc) { };

Geert Uytterhoeven

2016-10-26 07:31 UTC #1 lore raw

Quoted 26 lines · Magnus Damm

From: Magnus Damm <[email protected]>
For the DU to operate on R-Car Gen3 hardware a combination of DU and VSP devices are required. Since the DU driver also supports earlier generations hardware the VSP portion is enabled via Kconfig.
The arm64 defconfig is as of v4.9-rc1 having the DU driver enabled as a module, however this is not enough to support R-Car Gen3. In the current case of CONFIG_DRM_RCAR_VSP=n then the kernel crashes when loading the module. This patch is fixing that particular case.
In more detail, the crash triggers in drm_atomic_get_plane_state() when __drm_atomic_helper_set_config() passes NULL as crtc->primary.
This patch corrects this issue by failing to load the DU driver on R-Car Gen3 when VSP is not available.
Signed-off-by Magnus Damm <[email protected]>
---
 drivers/gpu/drm/rcar-du/rcar_du_vsp.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/gpu/drm/rcar-du/rcar_du_vsp.h 2016-10-26 00:01:12.920607110 +0900 void rcar_du_vsp_disable(struct rcar_du_ · line 73

Quoted 5 lines

drivers/gpu/drm/rcar-du/rcar_du_vsp.h 2016-10-26 00:01:12.920607110 +0900 renamed from drivers/gpu/drm/rcar-du/rcar_du_vsp.h +1 -1

@@ -70,7 +70,7 @@ void rcar_du_vsp_disable(struct rcar_du_
 void rcar_du_vsp_atomic_begin(struct rcar_du_crtc *crtc);
 void rcar_du_vsp_atomic_flush(struct rcar_du_crtc *crtc);
 #else
-static inline int rcar_du_vsp_init(struct rcar_du_vsp *vsp) { return 0; };
+static inline int rcar_du_vsp_init(struct rcar_du_vsp *vsp) { return -ENXIO; };

-ENODEV sounds more appropriate

drivers/gpu/drm/rcar-du/rcar_du_vsp.h 2016-10-26 00:01:12.920607110 +0900 void rcar_du_vsp_disable(struct rcar_du_ · line 76

Quoted 3 lines

drivers/gpu/drm/rcar-du/rcar_du_vsp.h 2016-10-26 00:01:12.920607110 +0900 renamed from drivers/gpu/drm/rcar-du/rcar_du_vsp.h +0 -0
 static inline void rcar_du_vsp_enable(struct rcar_du_crtc *crtc) { };
 static inline void rcar_du_vsp_disable(struct rcar_du_crtc *crtc) { };
 static inline void rcar_du_vsp_atomic_begin(struct rcar_du_crtc *crtc) { };

Alternatively, DRM_RCAR_DU can force DRM_RCAR_VSP to y if ARCH_R8A7795 or ARCH_R8A7796 is enabled.

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Magnus Damm

author maintainer

2016-10-26 09:13 UTC #2 lore raw

Hi Geert,

Quoted 24 lines · Geert Uytterhoeven

Quoted 23 lines · Magnus Damm
From: Magnus Damm <[email protected]>
For the DU to operate on R-Car Gen3 hardware a combination of DU and VSP devices are required. Since the DU driver also supports earlier generations hardware the VSP portion is enabled via Kconfig.
The arm64 defconfig is as of v4.9-rc1 having the DU driver enabled as a module, however this is not enough to support R-Car Gen3. In the current case of CONFIG_DRM_RCAR_VSP=n then the kernel crashes when loading the module. This patch is fixing that particular case.
In more detail, the crash triggers in drm_atomic_get_plane_state() when __drm_atomic_helper_set_config() passes NULL as crtc->primary.
This patch corrects this issue by failing to load the DU driver on R-Car Gen3 when VSP is not available.
Signed-off-by Magnus Damm <[email protected]>
---
 drivers/gpu/drm/rcar-du/rcar_du_vsp.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Quoted 10 lines

drivers/gpu/drm/rcar-du/rcar_du_vsp.h 2016-10-26 00:01:12.920607110 +0900 void rcar_du_vsp_disable(struct rcar_du_ · line 73
Quoted 8 lines
drivers/gpu/drm/rcar-du/rcar_du_vsp.h 2016-10-26 00:01:12.920607110 +0900 renamed from drivers/gpu/drm/rcar-du/rcar_du_vsp.h +1 -1
@@ -70,7 +70,7 @@ void rcar_du_vsp_disable(struct rcar_du_
 void rcar_du_vsp_atomic_begin(struct rcar_du_crtc *crtc);
 void rcar_du_vsp_atomic_flush(struct rcar_du_crtc *crtc);
 #else
-static inline int rcar_du_vsp_init(struct rcar_du_vsp *vsp) { return 0; };
+static inline int rcar_du_vsp_init(struct rcar_du_vsp *vsp) { return -ENXIO; };
-ENODEV sounds more appropriate

Ok, however -ENXIO is the same error code as the DU driver currently returns when dealing with the VSP and not finding DT nodes.

To avoid dealing with this again I would prefer skipping per-driver Kconfig options entirely, for instance something like this:

[PATCH/RFC] Simplify Gen3 DU and VSP Kconfig bits
https://www.spinics.net/lists/linux-sh/msg45978.html

Quoted 6 lines

drivers/gpu/drm/rcar-du/rcar_du_vsp.h 2016-10-26 00:01:12.920607110 +0900 void rcar_du_vsp_disable(struct rcar_du_ · line 76
Quoted 3 lines
drivers/gpu/drm/rcar-du/rcar_du_vsp.h 2016-10-26 00:01:12.920607110 +0900 renamed from drivers/gpu/drm/rcar-du/rcar_du_vsp.h +0 -0
 static inline void rcar_du_vsp_enable(struct rcar_du_crtc *crtc) { };
 static inline void rcar_du_vsp_disable(struct rcar_du_crtc *crtc) { };
 static inline void rcar_du_vsp_atomic_begin(struct rcar_du_crtc *crtc) { };
Alternatively, DRM_RCAR_DU can force DRM_RCAR_VSP to y if ARCH_R8A7795 or ARCH_R8A7796 is enabled.

The DU driver has symbol dependencies on VSP and FCP driver code (V4L2), so we need to take the state of driver support for those modules into consideration as well instead of only depending on SoC-specific Kconfig symbols.

Quoted 6 lines · Geert Uytterhoeven

Quoted 2 lines · Magnus Damm
The arm64 defconfig is as of v4.9-rc1 having the DU driver enabled as a module, however this is not enough to support R-Car Gen3. In
Nope, arm64 defconfig on v4.9-rc1:
# CONFIG_DRM_RCAR_DU is not set

Right, you are correct. Sorry for the noise. It seems that I was using renesas-drivers-2016-10-18-v4.9-rc1 - not mainline v4.9-rc1.

Without this fix the driver still crashes, but not due to defconfig in v4.9-rc1.

Thanks,

/ magnus

Laurent Pinchart

2016-12-06 14:06 UTC #3 lore raw

Hello,

Quoted 30 lines · On Wednesday 26 Oct 2016 18:13:23 Magnus Damm wrote:

Quoted 29 lines · Geert Uytterhoeven
Quoted 28 lines · Magnus Damm
From: Magnus Damm <[email protected]>
For the DU to operate on R-Car Gen3 hardware a combination of DU and VSP devices are required. Since the DU driver also supports earlier generations hardware the VSP portion is enabled via Kconfig.
The arm64 defconfig is as of v4.9-rc1 having the DU driver enabled as a module, however this is not enough to support R-Car Gen3. In the current case of CONFIG_DRM_RCAR_VSP=n then the kernel crashes when loading the module. This patch is fixing that particular case.
In more detail, the crash triggers in drm_atomic_get_plane_state() when __drm_atomic_helper_set_config() passes NULL as crtc->primary.
This patch corrects this issue by failing to load the DU driver on R-Car Gen3 when VSP is not available.
Signed-off-by Magnus Damm <[email protected]>
---
 drivers/gpu/drm/rcar-du/rcar_du_vsp.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/gpu/drm/rcar-du/rcar_du_vsp.h 2016-10-26 renamed from drivers/gpu/drm/rcar-du/rcar_du_vsp.h +0 -0
00:01:12.920607110 +0900 @@ -70,7 +70,7 @@ void
rcar_du_vsp_disable(struct rcar_du_

Quoted 8 lines

Quoted 8 lines
drivers/gpu/drm/rcar-du/rcar_du_vsp.h 2016-10-26
Quoted 8 lines
void rcar_du_vsp_atomic_begin(struct rcar_du_crtc *crtc); void rcar_du_vsp_atomic_flush(struct rcar_du_crtc *crtc); #else
-static inline int rcar_du_vsp_init(struct rcar_du_vsp *vsp) { return 0; };
drivers/gpu/drm/rcar-du/rcar_du_vsp.h 2016-10-26 renamed from drivers/gpu/drm/rcar-du/rcar_du_vsp.h +1 -1
+static inline int rcar_du_vsp_init(struct rcar_du_vsp *vsp) { return
-ENXIO; };

With this patch applied the DU will fail to probe on Gen2 if DRM_RCAR_VSP is disabled.

DRM_RCAR_DU should instead depend on VIDEO_RENESAS_VSP1 (with DRM_RCAR_VSP always set) on Gen3.

Quoted 37 lines

Quoted 1 line
-ENODEV sounds more appropriate
Ok, however -ENXIO is the same error code as the DU driver currently returns when dealing with the VSP and not finding DT nodes.
To avoid dealing with this again I would prefer skipping per-driver Kconfig options entirely, for instance something like this:
[PATCH/RFC] Simplify Gen3 DU and VSP Kconfig bits
https://www.spinics.net/lists/linux-sh/msg45978.html
Quoted 7 lines
Quoted 4 lines
static inline void rcar_du_vsp_enable(struct rcar_du_crtc *crtc) { }; static inline void rcar_du_vsp_disable(struct rcar_du_crtc *crtc) { }; static inline void rcar_du_vsp_atomic_begin(struct rcar_du_crtc *crtc) { };
Alternatively, DRM_RCAR_DU can force DRM_RCAR_VSP to y if ARCH_R8A7795 or ARCH_R8A7796 is enabled.
The DU driver has symbol dependencies on VSP and FCP driver code (V4L2), so we need to take the state of driver support for those modules into consideration as well instead of only depending on SoC-specific Kconfig symbols.
Quoted 6 lines · Geert Uytterhoeven
Quoted 2 lines · Magnus Damm
The arm64 defconfig is as of v4.9-rc1 having the DU driver enabled as a module, however this is not enough to support R-Car Gen3. In
Nope, arm64 defconfig on v4.9-rc1:
# CONFIG_DRM_RCAR_DU is not set
Right, you are correct. Sorry for the noise. It seems that I was using renesas-drivers-2016-10-18-v4.9-rc1 - not mainline v4.9-rc1.
Without this fix the driver still crashes, but not due to defconfig in v4.9-rc1.

Signature · 8 lines

Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Laurent Pinchart

2016-12-14 22:23 UTC #4 lore raw

Hi Magnus,

Quoted 44 lines · On Tuesday 06 Dec 2016 16:07:02 Laurent Pinchart wrote:

Quoted 34 lines · On Wednesday 26 Oct 2016 18:13:23 Magnus Damm wrote:
Quoted 33 lines · Geert Uytterhoeven
Quoted 32 lines · Magnus Damm
From: Magnus Damm <[email protected]>
For the DU to operate on R-Car Gen3 hardware a combination of DU and VSP devices are required. Since the DU driver also supports earlier generations hardware the VSP portion is enabled via Kconfig.
The arm64 defconfig is as of v4.9-rc1 having the DU driver enabled as a module, however this is not enough to support R-Car Gen3. In the current case of CONFIG_DRM_RCAR_VSP=n then the kernel crashes when loading the module. This patch is fixing that particular case.
In more detail, the crash triggers in drm_atomic_get_plane_state() when __drm_atomic_helper_set_config() passes NULL as crtc->primary.
This patch corrects this issue by failing to load the DU driver on R-Car Gen3 when VSP is not available.
Signed-off-by Magnus Damm <[email protected]>
---
 drivers/gpu/drm/rcar-du/rcar_du_vsp.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/gpu/drm/rcar-du/rcar_du_vsp.h 2016-10-26 renamed from drivers/gpu/drm/rcar-du/rcar_du_vsp.h +0 -0
00:01:12.920607110 +0900 @@ -70,7 +70,7 @@ void
rcar_du_vsp_disable(struct rcar_du_
void rcar_du_vsp_atomic_begin(struct rcar_du_crtc *crtc); void rcar_du_vsp_atomic_flush(struct rcar_du_crtc *crtc); #else
Quoted 5 lines
Quoted 5 lines
drivers/gpu/drm/rcar-du/rcar_du_vsp.h 2016-10-26
Quoted 5 lines
-static inline int rcar_du_vsp_init(struct rcar_du_vsp *vsp) { return 0;
};
drivers/gpu/drm/rcar-du/rcar_du_vsp.h 2016-10-26 renamed from drivers/gpu/drm/rcar-du/rcar_du_vsp.h +1 -1
+static inline int rcar_du_vsp_init(struct rcar_du_vsp *vsp) { return
-ENXIO; };
With this patch applied the DU will fail to probe on Gen2 if DRM_RCAR_VSP is disabled.

Quoted 2 lines

DRM_RCAR_DU should instead depend on VIDEO_RENESAS_VSP1 (with DRM_RCAR_VSP always set) on Gen3.

Do you plan to work on a v2 that fixes this in Kconfig ? I gave it a quick try but ran into a Kconfig circular dependency that I don't have time to debug now.

Quoted 38 lines

Quoted 38 lines
Quoted 1 line
-ENODEV sounds more appropriate
Ok, however -ENXIO is the same error code as the DU driver currently returns when dealing with the VSP and not finding DT nodes.
To avoid dealing with this again I would prefer skipping per-driver Kconfig options entirely, for instance something like this:
[PATCH/RFC] Simplify Gen3 DU and VSP Kconfig bits
https://www.spinics.net/lists/linux-sh/msg45978.html
Quoted 8 lines
Quoted 5 lines
static inline void rcar_du_vsp_enable(struct rcar_du_crtc *crtc) { }; static inline void rcar_du_vsp_disable(struct rcar_du_crtc *crtc) { }; static inline void rcar_du_vsp_atomic_begin(struct rcar_du_crtc *crtc) {
};
Alternatively, DRM_RCAR_DU can force DRM_RCAR_VSP to y if ARCH_R8A7795 or ARCH_R8A7796 is enabled.
The DU driver has symbol dependencies on VSP and FCP driver code (V4L2), so we need to take the state of driver support for those modules into consideration as well instead of only depending on SoC-specific Kconfig symbols.
Quoted 6 lines · Geert Uytterhoeven
Quoted 2 lines · Magnus Damm
The arm64 defconfig is as of v4.9-rc1 having the DU driver enabled as a module, however this is not enough to support R-Car Gen3. In
Nope, arm64 defconfig on v4.9-rc1:
# CONFIG_DRM_RCAR_DU is not set
Right, you are correct. Sorry for the noise. It seems that I was using renesas-drivers-2016-10-18-v4.9-rc1 - not mainline v4.9-rc1.
Without this fix the driver still crashes, but not due to defconfig in v4.9-rc1.

Signature · 3 lines

Regards,

Laurent Pinchart

Geert Uytterhoeven

2016-10-26 07:33 UTC #5 lore raw

In reply to Magnus Damm

Quoted 2 lines · Magnus Damm

The arm64 defconfig is as of v4.9-rc1 having the DU driver enabled as a module, however this is not enough to support R-Car Gen3. In

Nope, arm64 defconfig on v4.9-rc1:
# CONFIG_DRM_RCAR_DU is not set

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds