[PATCH v3 0/4] Fixes to bridge/panel and ingenic-drm

[PATCH v3 0/4] Fixes to bridge/panel and ingenic-drm

[Paul Cercueil]

Hi,

Here are three independent fixes. The first one addresses a
use-after-free in bridge/panel.c; the second one addresses a
use-after-free in the ingenic-drm driver; finally, the third one makes
the ingenic-drm driver work again on older Ingenic SoCs.

Changes from v2:
- patch [1/4] added a FIXME.
- patch [2/4] is new. It introduces a drmm_plain_simple_encoder_alloc()
  macro that will be used in patch [3/4].
- patch [3/4] uses the macro introduced in patch [2/4].
- patch [4/4] is unmodified.

Note to linux-stable guys: patch [v2 2/3] will only apply on the current
drm-misc-next branch, to fix it for v5.11 and older kernels, use the V1
of that patch.

Cheers,
-Paul

Paul Cercueil (4):
  drm: bridge/panel: Cleanup connector on bridge detach
  drm/simple_kms_helper: Add macro drmm_plain_simple_encoder_alloc()
  drm/ingenic: Register devm action to cleanup encoders
  drm/ingenic: Fix non-OSD mode

 drivers/gpu/drm/bridge/panel.c            | 12 +++++++++++
 drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 26 +++++++++++------------
 include/drm/drm_simple_kms_helper.h       | 17 +++++++++++++++
 3 files changed, 42 insertions(+), 13 deletions(-)

-- 
2.29.2

[PATCH v3 0/4] Fixes to bridge/panel and ingenic-drm

[Paul Cercueil]

Hi,

Here are three independent fixes. The first one addresses a
use-after-free in bridge/panel.c; the second one addresses a
use-after-free in the ingenic-drm driver; finally, the third one makes
the ingenic-drm driver work again on older Ingenic SoCs.

Changes from v2:
- patch [1/4] added a FIXME.
- patch [2/4] is new. It introduces a drmm_plain_simple_encoder_alloc()
  macro that will be used in patch [3/4].
- patch [3/4] uses the macro introduced in patch [2/4].
- patch [4/4] is unmodified.

Note to linux-stable guys: patch [v2 2/3] will only apply on the current
drm-misc-next branch, to fix it for v5.11 and older kernels, use the V1
of that patch.

Cheers,
-Paul

Paul Cercueil (4):
  drm: bridge/panel: Cleanup connector on bridge detach
  drm/simple_kms_helper: Add macro drmm_plain_simple_encoder_alloc()
  drm/ingenic: Register devm action to cleanup encoders
  drm/ingenic: Fix non-OSD mode

 drivers/gpu/drm/bridge/panel.c            | 12 +++++++++++
 drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 26 +++++++++++------------
 include/drm/drm_simple_kms_helper.h       | 17 +++++++++++++++
 3 files changed, 42 insertions(+), 13 deletions(-)

-- 
2.29.2

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v3 1/4] drm: bridge/panel: Cleanup connector on bridge detach

[Paul Cercueil]

If we don't call drm_connector_cleanup() manually in
panel_bridge_detach(), the connector will be cleaned up with the other
DRM objects in the call to drm_mode_config_cleanup(). However, since our
drm_connector is devm-allocated, by the time drm_mode_config_cleanup()
will be called, our connector will be long gone. Therefore, the
connector must be cleaned up when the bridge is detached to avoid
use-after-free conditions.

v2: Cleanup connector only if it was created

v3: Add FIXME

Fixes: 13dfc0540a57 ("drm/bridge: Refactor out the panel wrapper from the lvds-encoder bridge.")
Cc: <stable@vger.kernel.org> # 4.12+
Cc: Andrzej Hajda <a.hajda@samsung.com>
Cc: Neil Armstrong <narmstrong@baylibre.com>
Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
Cc: Jonas Karlman <jonas@kwiboo.se>
Cc: Jernej Skrabec <jernej.skrabec@siol.net>
Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---
 drivers/gpu/drm/bridge/panel.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c
index 0ddc37551194..5959e8183cd0 100644
--- a/drivers/gpu/drm/bridge/panel.c
+++ b/drivers/gpu/drm/bridge/panel.c
@@ -87,6 +87,18 @@ static int panel_bridge_attach(struct drm_bridge *bridge,
 
 static void panel_bridge_detach(struct drm_bridge *bridge)
 {
+	struct panel_bridge *panel_bridge = drm_bridge_to_panel_bridge(bridge);
+	struct drm_connector *connector = &panel_bridge->connector;
+
+	/*
+	 * Cleanup the connector if we know it was initialized.
+	 *
+	 * FIXME: This wouldn't be needed if the panel_bridge structure was
+	 * allocated with drmm_kzalloc(). This might be tricky since the
+	 * drm_device pointer can only be retrieved when the bridge is attached.
+	 */
+	if (!!panel_bridge->connector.dev)
+		drm_connector_cleanup(connector);
 }
 
 static void panel_bridge_pre_enable(struct drm_bridge *bridge)
-- 
2.29.2

[PATCH v3 1/4] drm: bridge/panel: Cleanup connector on bridge detach

[Paul Cercueil]

If we don't call drm_connector_cleanup() manually in
panel_bridge_detach(), the connector will be cleaned up with the other
DRM objects in the call to drm_mode_config_cleanup(). However, since our
drm_connector is devm-allocated, by the time drm_mode_config_cleanup()
will be called, our connector will be long gone. Therefore, the
connector must be cleaned up when the bridge is detached to avoid
use-after-free conditions.

v2: Cleanup connector only if it was created

v3: Add FIXME

Fixes: 13dfc0540a57 ("drm/bridge: Refactor out the panel wrapper from the lvds-encoder bridge.")
Cc: <stable@vger.kernel.org> # 4.12+
Cc: Andrzej Hajda <a.hajda@samsung.com>
Cc: Neil Armstrong <narmstrong@baylibre.com>
Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
Cc: Jonas Karlman <jonas@kwiboo.se>
Cc: Jernej Skrabec <jernej.skrabec@siol.net>
Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---
 drivers/gpu/drm/bridge/panel.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c
index 0ddc37551194..5959e8183cd0 100644
--- a/drivers/gpu/drm/bridge/panel.c
+++ b/drivers/gpu/drm/bridge/panel.c
@@ -87,6 +87,18 @@ static int panel_bridge_attach(struct drm_bridge *bridge,
 
 static void panel_bridge_detach(struct drm_bridge *bridge)
 {
+	struct panel_bridge *panel_bridge = drm_bridge_to_panel_bridge(bridge);
+	struct drm_connector *connector = &panel_bridge->connector;
+
+	/*
+	 * Cleanup the connector if we know it was initialized.
+	 *
+	 * FIXME: This wouldn't be needed if the panel_bridge structure was
+	 * allocated with drmm_kzalloc(). This might be tricky since the
+	 * drm_device pointer can only be retrieved when the bridge is attached.
+	 */
+	if (!!panel_bridge->connector.dev)
+		drm_connector_cleanup(connector);
 }
 
 static void panel_bridge_pre_enable(struct drm_bridge *bridge)
-- 
2.29.2

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v3 2/4] drm/simple_kms_helper: Add macro drmm_plain_simple_encoder_alloc()

[Paul Cercueil]

This performs the same operation as drmm_simple_encoder_alloc(), but
only allocates and returns a struct drm_encoder instance.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---
 include/drm/drm_simple_kms_helper.h | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/include/drm/drm_simple_kms_helper.h b/include/drm/drm_simple_kms_helper.h
index e6dbf3161c2f..f07e70303cfb 100644
--- a/include/drm/drm_simple_kms_helper.h
+++ b/include/drm/drm_simple_kms_helper.h
@@ -209,4 +209,21 @@ void *__drmm_simple_encoder_alloc(struct drm_device *dev, size_t size,
 					     offsetof(type, member), \
 					     encoder_type))
 
+/**
+ * drmm_plain_simple_encoder_alloc - Allocate and initialize a drm_encoder
+ *                                   struct with basic functionality.
+ * @dev: drm device
+ * @encoder_type: user visible type of the encoder
+ *
+ * This performs the same operation as drmm_simple_encoder_alloc(), but
+ * only allocates and returns a struct drm_encoder instance.
+ *
+ * Returns:
+ * Pointer to the new drm_encoder struct, or ERR_PTR on failure.
+ */
+#define drmm_plain_simple_encoder_alloc(dev, encoder_type) \
+	((struct drm_encoder *) \
+	 __drmm_simple_encoder_alloc(dev, sizeof(struct drm_encoder), \
+				     0, encoder_type))
+
 #endif /* __LINUX_DRM_SIMPLE_KMS_HELPER_H */
-- 
2.29.2

[PATCH v3 2/4] drm/simple_kms_helper: Add macro drmm_plain_simple_encoder_alloc()

[Paul Cercueil]

This performs the same operation as drmm_simple_encoder_alloc(), but
only allocates and returns a struct drm_encoder instance.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---
 include/drm/drm_simple_kms_helper.h | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/include/drm/drm_simple_kms_helper.h b/include/drm/drm_simple_kms_helper.h
index e6dbf3161c2f..f07e70303cfb 100644
--- a/include/drm/drm_simple_kms_helper.h
+++ b/include/drm/drm_simple_kms_helper.h
@@ -209,4 +209,21 @@ void *__drmm_simple_encoder_alloc(struct drm_device *dev, size_t size,
 					     offsetof(type, member), \
 					     encoder_type))
 
+/**
+ * drmm_plain_simple_encoder_alloc - Allocate and initialize a drm_encoder
+ *                                   struct with basic functionality.
+ * @dev: drm device
+ * @encoder_type: user visible type of the encoder
+ *
+ * This performs the same operation as drmm_simple_encoder_alloc(), but
+ * only allocates and returns a struct drm_encoder instance.
+ *
+ * Returns:
+ * Pointer to the new drm_encoder struct, or ERR_PTR on failure.
+ */
+#define drmm_plain_simple_encoder_alloc(dev, encoder_type) \
+	((struct drm_encoder *) \
+	 __drmm_simple_encoder_alloc(dev, sizeof(struct drm_encoder), \
+				     0, encoder_type))
+
 #endif /* __LINUX_DRM_SIMPLE_KMS_HELPER_H */
-- 
2.29.2

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v3 3/4] drm/ingenic: Register devm action to cleanup encoders

[Paul Cercueil]

Since the encoders have been devm-allocated, they will be freed way
before drm_mode_config_cleanup() is called. To avoid use-after-free
conditions, we then must ensure that drm_encoder_cleanup() is called
before the encoders are freed.

v2: Use the new __drmm_simple_encoder_alloc() function

v3: Use the new drmm_plain_simple_encoder_alloc() macro

Fixes: c369cb27c267 ("drm/ingenic: Support multiple panels/bridges")
Cc: <stable@vger.kernel.org> # 5.8+
Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---

Notes:
    Use the V1 of this patch to fix v5.11 and older kernels. This V3 only
    applies on the current drm-misc-next branch.

 drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
index 7bb31fbee29d..b23011c1c5d9 100644
--- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
+++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
@@ -1014,20 +1014,17 @@ static int ingenic_drm_bind(struct device *dev, bool has_components)
 			bridge = devm_drm_panel_bridge_add_typed(dev, panel,
 								 DRM_MODE_CONNECTOR_DPI);
 
-		encoder = devm_kzalloc(dev, sizeof(*encoder), GFP_KERNEL);
-		if (!encoder)
-			return -ENOMEM;
+		encoder = drmm_plain_simple_encoder_alloc(drm, DRM_MODE_ENCODER_DPI);
+		if (IS_ERR(encoder)) {
+			ret = PTR_ERR(encoder);
+			dev_err(dev, "Failed to init encoder: %d\n", ret);
+			return ret;
+		}
 
 		encoder->possible_crtcs = 1;
 
 		drm_encoder_helper_add(encoder, &ingenic_drm_encoder_helper_funcs);
 
-		ret = drm_simple_encoder_init(drm, encoder, DRM_MODE_ENCODER_DPI);
-		if (ret) {
-			dev_err(dev, "Failed to init encoder: %d\n", ret);
-			return ret;
-		}
-
 		ret = drm_bridge_attach(encoder, bridge, NULL, 0);
 		if (ret) {
 			dev_err(dev, "Unable to attach bridge\n");
-- 
2.29.2

[PATCH v3 3/4] drm/ingenic: Register devm action to cleanup encoders

[Paul Cercueil]

Since the encoders have been devm-allocated, they will be freed way
before drm_mode_config_cleanup() is called. To avoid use-after-free
conditions, we then must ensure that drm_encoder_cleanup() is called
before the encoders are freed.

v2: Use the new __drmm_simple_encoder_alloc() function

v3: Use the new drmm_plain_simple_encoder_alloc() macro

Fixes: c369cb27c267 ("drm/ingenic: Support multiple panels/bridges")
Cc: <stable@vger.kernel.org> # 5.8+
Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---

Notes:
    Use the V1 of this patch to fix v5.11 and older kernels. This V3 only
    applies on the current drm-misc-next branch.

 drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
index 7bb31fbee29d..b23011c1c5d9 100644
--- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
+++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
@@ -1014,20 +1014,17 @@ static int ingenic_drm_bind(struct device *dev, bool has_components)
 			bridge = devm_drm_panel_bridge_add_typed(dev, panel,
 								 DRM_MODE_CONNECTOR_DPI);
 
-		encoder = devm_kzalloc(dev, sizeof(*encoder), GFP_KERNEL);
-		if (!encoder)
-			return -ENOMEM;
+		encoder = drmm_plain_simple_encoder_alloc(drm, DRM_MODE_ENCODER_DPI);
+		if (IS_ERR(encoder)) {
+			ret = PTR_ERR(encoder);
+			dev_err(dev, "Failed to init encoder: %d\n", ret);
+			return ret;
+		}
 
 		encoder->possible_crtcs = 1;
 
 		drm_encoder_helper_add(encoder, &ingenic_drm_encoder_helper_funcs);
 
-		ret = drm_simple_encoder_init(drm, encoder, DRM_MODE_ENCODER_DPI);
-		if (ret) {
-			dev_err(dev, "Failed to init encoder: %d\n", ret);
-			return ret;
-		}
-
 		ret = drm_bridge_attach(encoder, bridge, NULL, 0);
 		if (ret) {
 			dev_err(dev, "Unable to attach bridge\n");
-- 
2.29.2

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v3 4/4] drm/ingenic: Fix non-OSD mode

[Paul Cercueil]

Even though the JZ4740 did not have the OSD mode, it had (according to
the documentation) two DMA channels, but there is absolutely no
information about how to select the second DMA channel.

Make the ingenic-drm driver work in non-OSD mode by using the
foreground0 plane (which is bound to the DMA0 channel) as the primary
plane, instead of the foreground1 plane, which is the primary plane
when in OSD mode.

Fixes: 3c9bea4ef32b ("drm/ingenic: Add support for OSD mode")
Cc: <stable@vger.kernel.org> # v5.8+
Signed-off-by: Paul Cercueil <paul@crapouillou.net>
Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
index b23011c1c5d9..59ce43862e16 100644
--- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
+++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
@@ -554,7 +554,7 @@ static void ingenic_drm_plane_atomic_update(struct drm_plane *plane,
 		height = state->src_h >> 16;
 		cpp = state->fb->format->cpp[0];
 
-		if (priv->soc_info->has_osd && plane->type == DRM_PLANE_TYPE_OVERLAY)
+		if (!priv->soc_info->has_osd || plane->type == DRM_PLANE_TYPE_OVERLAY)
 			hwdesc = &priv->dma_hwdescs->hwdesc_f0;
 		else
 			hwdesc = &priv->dma_hwdescs->hwdesc_f1;
@@ -826,6 +826,7 @@ static int ingenic_drm_bind(struct device *dev, bool has_components)
 	const struct jz_soc_info *soc_info;
 	struct ingenic_drm *priv;
 	struct clk *parent_clk;
+	struct drm_plane *primary;
 	struct drm_bridge *bridge;
 	struct drm_panel *panel;
 	struct drm_encoder *encoder;
@@ -940,9 +941,11 @@ static int ingenic_drm_bind(struct device *dev, bool has_components)
 	if (soc_info->has_osd)
 		priv->ipu_plane = drm_plane_from_index(drm, 0);
 
-	drm_plane_helper_add(&priv->f1, &ingenic_drm_plane_helper_funcs);
+	primary = priv->soc_info->has_osd ? &priv->f1 : &priv->f0;
 
-	ret = drm_universal_plane_init(drm, &priv->f1, 1,
+	drm_plane_helper_add(primary, &ingenic_drm_plane_helper_funcs);
+
+	ret = drm_universal_plane_init(drm, primary, 1,
 				       &ingenic_drm_primary_plane_funcs,
 				       priv->soc_info->formats_f1,
 				       priv->soc_info->num_formats_f1,
@@ -954,7 +957,7 @@ static int ingenic_drm_bind(struct device *dev, bool has_components)
 
 	drm_crtc_helper_add(&priv->crtc, &ingenic_drm_crtc_helper_funcs);
 
-	ret = drm_crtc_init_with_planes(drm, &priv->crtc, &priv->f1,
+	ret = drm_crtc_init_with_planes(drm, &priv->crtc, primary,
 					NULL, &ingenic_drm_crtc_funcs, NULL);
 	if (ret) {
 		dev_err(dev, "Failed to init CRTC: %i\n", ret);
-- 
2.29.2

[PATCH v3 4/4] drm/ingenic: Fix non-OSD mode

[Paul Cercueil]

Even though the JZ4740 did not have the OSD mode, it had (according to
the documentation) two DMA channels, but there is absolutely no
information about how to select the second DMA channel.

Make the ingenic-drm driver work in non-OSD mode by using the
foreground0 plane (which is bound to the DMA0 channel) as the primary
plane, instead of the foreground1 plane, which is the primary plane
when in OSD mode.

Fixes: 3c9bea4ef32b ("drm/ingenic: Add support for OSD mode")
Cc: <stable@vger.kernel.org> # v5.8+
Signed-off-by: Paul Cercueil <paul@crapouillou.net>
Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
index b23011c1c5d9..59ce43862e16 100644
--- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
+++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
@@ -554,7 +554,7 @@ static void ingenic_drm_plane_atomic_update(struct drm_plane *plane,
 		height = state->src_h >> 16;
 		cpp = state->fb->format->cpp[0];
 
-		if (priv->soc_info->has_osd && plane->type == DRM_PLANE_TYPE_OVERLAY)
+		if (!priv->soc_info->has_osd || plane->type == DRM_PLANE_TYPE_OVERLAY)
 			hwdesc = &priv->dma_hwdescs->hwdesc_f0;
 		else
 			hwdesc = &priv->dma_hwdescs->hwdesc_f1;
@@ -826,6 +826,7 @@ static int ingenic_drm_bind(struct device *dev, bool has_components)
 	const struct jz_soc_info *soc_info;
 	struct ingenic_drm *priv;
 	struct clk *parent_clk;
+	struct drm_plane *primary;
 	struct drm_bridge *bridge;
 	struct drm_panel *panel;
 	struct drm_encoder *encoder;
@@ -940,9 +941,11 @@ static int ingenic_drm_bind(struct device *dev, bool has_components)
 	if (soc_info->has_osd)
 		priv->ipu_plane = drm_plane_from_index(drm, 0);
 
-	drm_plane_helper_add(&priv->f1, &ingenic_drm_plane_helper_funcs);
+	primary = priv->soc_info->has_osd ? &priv->f1 : &priv->f0;
 
-	ret = drm_universal_plane_init(drm, &priv->f1, 1,
+	drm_plane_helper_add(primary, &ingenic_drm_plane_helper_funcs);
+
+	ret = drm_universal_plane_init(drm, primary, 1,
 				       &ingenic_drm_primary_plane_funcs,
 				       priv->soc_info->formats_f1,
 				       priv->soc_info->num_formats_f1,
@@ -954,7 +957,7 @@ static int ingenic_drm_bind(struct device *dev, bool has_components)
 
 	drm_crtc_helper_add(&priv->crtc, &ingenic_drm_crtc_helper_funcs);
 
-	ret = drm_crtc_init_with_planes(drm, &priv->crtc, &priv->f1,
+	ret = drm_crtc_init_with_planes(drm, &priv->crtc, primary,
 					NULL, &ingenic_drm_crtc_funcs, NULL);
 	if (ret) {
 		dev_err(dev, "Failed to init CRTC: %i\n", ret);
-- 
2.29.2

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 4/4] drm/ingenic: Fix non-OSD mode

[H. Nikolaus Schaller]

Hi Paul,
we observed the same issue on the jz4730 (which is almost identical
to the jz4740 wrt. LCDC) and our solution [1] was simpler.

It leaves the hwdesc f0 and f1 as they are and just takes f1 for really
programming the first DMA descriptor if there is no OSD.

We have tested on jz4730 and jz4780.

Maybe you want to consider that. Then I can officially post it.

[1] https://github.com/goldelico/letux-kernel/commit/3be1de5fdabf2cc1c17f198ded3328cc6e4b9844

> Am 24.01.2021 um 09:55 schrieb Paul Cercueil <paul@crapouillou.net>:
> 
> Even though the JZ4740 did not have the OSD mode, it had (according to
> the documentation) two DMA channels, but there is absolutely no
> information about how to select the second DMA channel.
> 
> Make the ingenic-drm driver work in non-OSD mode by using the
> foreground0 plane (which is bound to the DMA0 channel) as the primary
> plane, instead of the foreground1 plane, which is the primary plane
> when in OSD mode.
> 
> Fixes: 3c9bea4ef32b ("drm/ingenic: Add support for OSD mode")
> Cc: <stable@vger.kernel.org> # v5.8+
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
> drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 11 +++++++----
> 1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> index b23011c1c5d9..59ce43862e16 100644
> --- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> +++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> @@ -554,7 +554,7 @@ static void ingenic_drm_plane_atomic_update(struct drm_plane *plane,
> 		height = state->src_h >> 16;
> 		cpp = state->fb->format->cpp[0];
> 
> -		if (priv->soc_info->has_osd && plane->type == DRM_PLANE_TYPE_OVERLAY)
> +		if (!priv->soc_info->has_osd || plane->type == DRM_PLANE_TYPE_OVERLAY)
> 			hwdesc = &priv->dma_hwdescs->hwdesc_f0;
> 		else
> 			hwdesc = &priv->dma_hwdescs->hwdesc_f1;

we just replace this with

                if (priv->soc_info->has_osd && plane->type != DRM_PLANE_TYPE_OVERLAY)
                        hwdesc = &priv->dma_hwdescs->hwdesc_f1;
                else
                        hwdesc = &priv->dma_hwdescs->hwdesc_f0;

and the remainder can stay as is.

> @@ -826,6 +826,7 @@ static int ingenic_drm_bind(struct device *dev, bool has_components)
> 	const struct jz_soc_info *soc_info;
> 	struct ingenic_drm *priv;
> 	struct clk *parent_clk;
> +	struct drm_plane *primary;
> 	struct drm_bridge *bridge;
> 	struct drm_panel *panel;
> 	struct drm_encoder *encoder;
> @@ -940,9 +941,11 @@ static int ingenic_drm_bind(struct device *dev, bool has_components)
> 	if (soc_info->has_osd)
> 		priv->ipu_plane = drm_plane_from_index(drm, 0);
> 
> -	drm_plane_helper_add(&priv->f1, &ingenic_drm_plane_helper_funcs);
> +	primary = priv->soc_info->has_osd ? &priv->f1 : &priv->f0;
> 
> -	ret = drm_universal_plane_init(drm, &priv->f1, 1,
> +	drm_plane_helper_add(primary, &ingenic_drm_plane_helper_funcs);
> +
> +	ret = drm_universal_plane_init(drm, primary, 1,
> 				       &ingenic_drm_primary_plane_funcs,
> 				       priv->soc_info->formats_f1,
> 				       priv->soc_info->num_formats_f1,
> @@ -954,7 +957,7 @@ static int ingenic_drm_bind(struct device *dev, bool has_components)
> 
> 	drm_crtc_helper_add(&priv->crtc, &ingenic_drm_crtc_helper_funcs);
> 
> -	ret = drm_crtc_init_with_planes(drm, &priv->crtc, &priv->f1,
> +	ret = drm_crtc_init_with_planes(drm, &priv->crtc, primary,
> 					NULL, &ingenic_drm_crtc_funcs, NULL);
> 	if (ret) {
> 		dev_err(dev, "Failed to init CRTC: %i\n", ret);
> -- 
> 2.29.2
> 

BR and thanks,
Nikolaus

Re: [PATCH v3 4/4] drm/ingenic: Fix non-OSD mode

[H. Nikolaus Schaller]

Hi Paul,
we observed the same issue on the jz4730 (which is almost identical
to the jz4740 wrt. LCDC) and our solution [1] was simpler.

It leaves the hwdesc f0 and f1 as they are and just takes f1 for really
programming the first DMA descriptor if there is no OSD.

We have tested on jz4730 and jz4780.

Maybe you want to consider that. Then I can officially post it.

[1] https://github.com/goldelico/letux-kernel/commit/3be1de5fdabf2cc1c17f198ded3328cc6e4b9844

> Am 24.01.2021 um 09:55 schrieb Paul Cercueil <paul@crapouillou.net>:
> 
> Even though the JZ4740 did not have the OSD mode, it had (according to
> the documentation) two DMA channels, but there is absolutely no
> information about how to select the second DMA channel.
> 
> Make the ingenic-drm driver work in non-OSD mode by using the
> foreground0 plane (which is bound to the DMA0 channel) as the primary
> plane, instead of the foreground1 plane, which is the primary plane
> when in OSD mode.
> 
> Fixes: 3c9bea4ef32b ("drm/ingenic: Add support for OSD mode")
> Cc: <stable@vger.kernel.org> # v5.8+
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
> drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 11 +++++++----
> 1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> index b23011c1c5d9..59ce43862e16 100644
> --- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> +++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> @@ -554,7 +554,7 @@ static void ingenic_drm_plane_atomic_update(struct drm_plane *plane,
> 		height = state->src_h >> 16;
> 		cpp = state->fb->format->cpp[0];
> 
> -		if (priv->soc_info->has_osd && plane->type == DRM_PLANE_TYPE_OVERLAY)
> +		if (!priv->soc_info->has_osd || plane->type == DRM_PLANE_TYPE_OVERLAY)
> 			hwdesc = &priv->dma_hwdescs->hwdesc_f0;
> 		else
> 			hwdesc = &priv->dma_hwdescs->hwdesc_f1;

we just replace this with

                if (priv->soc_info->has_osd && plane->type != DRM_PLANE_TYPE_OVERLAY)
                        hwdesc = &priv->dma_hwdescs->hwdesc_f1;
                else
                        hwdesc = &priv->dma_hwdescs->hwdesc_f0;

and the remainder can stay as is.

> @@ -826,6 +826,7 @@ static int ingenic_drm_bind(struct device *dev, bool has_components)
> 	const struct jz_soc_info *soc_info;
> 	struct ingenic_drm *priv;
> 	struct clk *parent_clk;
> +	struct drm_plane *primary;
> 	struct drm_bridge *bridge;
> 	struct drm_panel *panel;
> 	struct drm_encoder *encoder;
> @@ -940,9 +941,11 @@ static int ingenic_drm_bind(struct device *dev, bool has_components)
> 	if (soc_info->has_osd)
> 		priv->ipu_plane = drm_plane_from_index(drm, 0);
> 
> -	drm_plane_helper_add(&priv->f1, &ingenic_drm_plane_helper_funcs);
> +	primary = priv->soc_info->has_osd ? &priv->f1 : &priv->f0;
> 
> -	ret = drm_universal_plane_init(drm, &priv->f1, 1,
> +	drm_plane_helper_add(primary, &ingenic_drm_plane_helper_funcs);
> +
> +	ret = drm_universal_plane_init(drm, primary, 1,
> 				       &ingenic_drm_primary_plane_funcs,
> 				       priv->soc_info->formats_f1,
> 				       priv->soc_info->num_formats_f1,
> @@ -954,7 +957,7 @@ static int ingenic_drm_bind(struct device *dev, bool has_components)
> 
> 	drm_crtc_helper_add(&priv->crtc, &ingenic_drm_crtc_helper_funcs);
> 
> -	ret = drm_crtc_init_with_planes(drm, &priv->crtc, &priv->f1,
> +	ret = drm_crtc_init_with_planes(drm, &priv->crtc, primary,
> 					NULL, &ingenic_drm_crtc_funcs, NULL);
> 	if (ret) {
> 		dev_err(dev, "Failed to init CRTC: %i\n", ret);
> -- 
> 2.29.2
> 

BR and thanks,
Nikolaus

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 4/4] drm/ingenic: Fix non-OSD mode

[Paul Cercueil]

Hi Nikolaus,

Le dim. 24 janv. 2021 à 10:30, H. Nikolaus Schaller 
<hns@goldelico.com> a écrit :
> Hi Paul,
> we observed the same issue on the jz4730 (which is almost identical
> to the jz4740 wrt. LCDC) and our solution [1] was simpler.
> 
> It leaves the hwdesc f0 and f1 as they are and just takes f1 for 
> really
> programming the first DMA descriptor if there is no OSD.

Disagreed. With your solution, it ends up using priv->f1 plane with 
hwdesc_f0. That's very confusing.

> We have tested on jz4730 and jz4780.

Could I get a tested-by then? :)

Cheers,
-Paul

> Maybe you want to consider that. Then I can officially post it.
> 
> [1] 
> https://github.com/goldelico/letux-kernel/commit/3be1de5fdabf2cc1c17f198ded3328cc6e4b9844
> 
>>  Am 24.01.2021 um 09:55 schrieb Paul Cercueil <paul@crapouillou.net>:
>> 
>>  Even though the JZ4740 did not have the OSD mode, it had (according 
>> to
>>  the documentation) two DMA channels, but there is absolutely no
>>  information about how to select the second DMA channel.
>> 
>>  Make the ingenic-drm driver work in non-OSD mode by using the
>>  foreground0 plane (which is bound to the DMA0 channel) as the 
>> primary
>>  plane, instead of the foreground1 plane, which is the primary plane
>>  when in OSD mode.
>> 
>>  Fixes: 3c9bea4ef32b ("drm/ingenic: Add support for OSD mode")
>>  Cc: <stable@vger.kernel.org> # v5.8+
>>  Signed-off-by: Paul Cercueil <paul@crapouillou.net>
>>  Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>>  ---
>>  drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 11 +++++++----
>>  1 file changed, 7 insertions(+), 4 deletions(-)
>> 
>>  diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c 
>> b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
>>  index b23011c1c5d9..59ce43862e16 100644
>>  --- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
>>  +++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
>>  @@ -554,7 +554,7 @@ static void 
>> ingenic_drm_plane_atomic_update(struct drm_plane *plane,
>>  		height = state->src_h >> 16;
>>  		cpp = state->fb->format->cpp[0];
>> 
>>  -		if (priv->soc_info->has_osd && plane->type == 
>> DRM_PLANE_TYPE_OVERLAY)
>>  +		if (!priv->soc_info->has_osd || plane->type == 
>> DRM_PLANE_TYPE_OVERLAY)
>>  			hwdesc = &priv->dma_hwdescs->hwdesc_f0;
>>  		else
>>  			hwdesc = &priv->dma_hwdescs->hwdesc_f1;
> 
> we just replace this with
> 
>                 if (priv->soc_info->has_osd && plane->type != 
> DRM_PLANE_TYPE_OVERLAY)
>                         hwdesc = &priv->dma_hwdescs->hwdesc_f1;
>                 else
>                         hwdesc = &priv->dma_hwdescs->hwdesc_f0;
> 
> and the remainder can stay as is.
> 
>>  @@ -826,6 +826,7 @@ static int ingenic_drm_bind(struct device *dev, 
>> bool has_components)
>>  	const struct jz_soc_info *soc_info;
>>  	struct ingenic_drm *priv;
>>  	struct clk *parent_clk;
>>  +	struct drm_plane *primary;
>>  	struct drm_bridge *bridge;
>>  	struct drm_panel *panel;
>>  	struct drm_encoder *encoder;
>>  @@ -940,9 +941,11 @@ static int ingenic_drm_bind(struct device 
>> *dev, bool has_components)
>>  	if (soc_info->has_osd)
>>  		priv->ipu_plane = drm_plane_from_index(drm, 0);
>> 
>>  -	drm_plane_helper_add(&priv->f1, &ingenic_drm_plane_helper_funcs);
>>  +	primary = priv->soc_info->has_osd ? &priv->f1 : &priv->f0;
>> 
>>  -	ret = drm_universal_plane_init(drm, &priv->f1, 1,
>>  +	drm_plane_helper_add(primary, &ingenic_drm_plane_helper_funcs);
>>  +
>>  +	ret = drm_universal_plane_init(drm, primary, 1,
>>  				       &ingenic_drm_primary_plane_funcs,
>>  				       priv->soc_info->formats_f1,
>>  				       priv->soc_info->num_formats_f1,
>>  @@ -954,7 +957,7 @@ static int ingenic_drm_bind(struct device *dev, 
>> bool has_components)
>> 
>>  	drm_crtc_helper_add(&priv->crtc, &ingenic_drm_crtc_helper_funcs);
>> 
>>  -	ret = drm_crtc_init_with_planes(drm, &priv->crtc, &priv->f1,
>>  +	ret = drm_crtc_init_with_planes(drm, &priv->crtc, primary,
>>  					NULL, &ingenic_drm_crtc_funcs, NULL);
>>  	if (ret) {
>>  		dev_err(dev, "Failed to init CRTC: %i\n", ret);
>>  --
>>  2.29.2
>> 
> 
> BR and thanks,
> Nikolaus
> 

Re: [PATCH v3 4/4] drm/ingenic: Fix non-OSD mode

[Paul Cercueil]

Hi Nikolaus,

Le dim. 24 janv. 2021 à 10:30, H. Nikolaus Schaller 
<hns@goldelico.com> a écrit :
> Hi Paul,
> we observed the same issue on the jz4730 (which is almost identical
> to the jz4740 wrt. LCDC) and our solution [1] was simpler.
> 
> It leaves the hwdesc f0 and f1 as they are and just takes f1 for 
> really
> programming the first DMA descriptor if there is no OSD.

Disagreed. With your solution, it ends up using priv->f1 plane with 
hwdesc_f0. That's very confusing.

> We have tested on jz4730 and jz4780.

Could I get a tested-by then? :)

Cheers,
-Paul

> Maybe you want to consider that. Then I can officially post it.
> 
> [1] 
> https://github.com/goldelico/letux-kernel/commit/3be1de5fdabf2cc1c17f198ded3328cc6e4b9844
> 
>>  Am 24.01.2021 um 09:55 schrieb Paul Cercueil <paul@crapouillou.net>:
>> 
>>  Even though the JZ4740 did not have the OSD mode, it had (according 
>> to
>>  the documentation) two DMA channels, but there is absolutely no
>>  information about how to select the second DMA channel.
>> 
>>  Make the ingenic-drm driver work in non-OSD mode by using the
>>  foreground0 plane (which is bound to the DMA0 channel) as the 
>> primary
>>  plane, instead of the foreground1 plane, which is the primary plane
>>  when in OSD mode.
>> 
>>  Fixes: 3c9bea4ef32b ("drm/ingenic: Add support for OSD mode")
>>  Cc: <stable@vger.kernel.org> # v5.8+
>>  Signed-off-by: Paul Cercueil <paul@crapouillou.net>
>>  Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>>  ---
>>  drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 11 +++++++----
>>  1 file changed, 7 insertions(+), 4 deletions(-)
>> 
>>  diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c 
>> b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
>>  index b23011c1c5d9..59ce43862e16 100644
>>  --- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
>>  +++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
>>  @@ -554,7 +554,7 @@ static void 
>> ingenic_drm_plane_atomic_update(struct drm_plane *plane,
>>  		height = state->src_h >> 16;
>>  		cpp = state->fb->format->cpp[0];
>> 
>>  -		if (priv->soc_info->has_osd && plane->type == 
>> DRM_PLANE_TYPE_OVERLAY)
>>  +		if (!priv->soc_info->has_osd || plane->type == 
>> DRM_PLANE_TYPE_OVERLAY)
>>  			hwdesc = &priv->dma_hwdescs->hwdesc_f0;
>>  		else
>>  			hwdesc = &priv->dma_hwdescs->hwdesc_f1;
> 
> we just replace this with
> 
>                 if (priv->soc_info->has_osd && plane->type != 
> DRM_PLANE_TYPE_OVERLAY)
>                         hwdesc = &priv->dma_hwdescs->hwdesc_f1;
>                 else
>                         hwdesc = &priv->dma_hwdescs->hwdesc_f0;
> 
> and the remainder can stay as is.
> 
>>  @@ -826,6 +826,7 @@ static int ingenic_drm_bind(struct device *dev, 
>> bool has_components)
>>  	const struct jz_soc_info *soc_info;
>>  	struct ingenic_drm *priv;
>>  	struct clk *parent_clk;
>>  +	struct drm_plane *primary;
>>  	struct drm_bridge *bridge;
>>  	struct drm_panel *panel;
>>  	struct drm_encoder *encoder;
>>  @@ -940,9 +941,11 @@ static int ingenic_drm_bind(struct device 
>> *dev, bool has_components)
>>  	if (soc_info->has_osd)
>>  		priv->ipu_plane = drm_plane_from_index(drm, 0);
>> 
>>  -	drm_plane_helper_add(&priv->f1, &ingenic_drm_plane_helper_funcs);
>>  +	primary = priv->soc_info->has_osd ? &priv->f1 : &priv->f0;
>> 
>>  -	ret = drm_universal_plane_init(drm, &priv->f1, 1,
>>  +	drm_plane_helper_add(primary, &ingenic_drm_plane_helper_funcs);
>>  +
>>  +	ret = drm_universal_plane_init(drm, primary, 1,
>>  				       &ingenic_drm_primary_plane_funcs,
>>  				       priv->soc_info->formats_f1,
>>  				       priv->soc_info->num_formats_f1,
>>  @@ -954,7 +957,7 @@ static int ingenic_drm_bind(struct device *dev, 
>> bool has_components)
>> 
>>  	drm_crtc_helper_add(&priv->crtc, &ingenic_drm_crtc_helper_funcs);
>> 
>>  -	ret = drm_crtc_init_with_planes(drm, &priv->crtc, &priv->f1,
>>  +	ret = drm_crtc_init_with_planes(drm, &priv->crtc, primary,
>>  					NULL, &ingenic_drm_crtc_funcs, NULL);
>>  	if (ret) {
>>  		dev_err(dev, "Failed to init CRTC: %i\n", ret);
>>  --
>>  2.29.2
>> 
> 
> BR and thanks,
> Nikolaus
> 


_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 4/4] drm/ingenic: Fix non-OSD mode

[H. Nikolaus Schaller]

Hi Paul,

> Am 24.01.2021 um 10:43 schrieb Paul Cercueil <paul@crapouillou.net>:
> 
> Hi Nikolaus,
> 
> Le dim. 24 janv. 2021 à 10:30, H. Nikolaus Schaller <hns@goldelico.com> a écrit :
>> Hi Paul,
>> we observed the same issue on the jz4730 (which is almost identical
>> to the jz4740 wrt. LCDC) and our solution [1] was simpler.
>> It leaves the hwdesc f0 and f1 as they are and just takes f1 for really
>> programming the first DMA descriptor if there is no OSD.
> 
> Disagreed. With your solution, it ends up using priv->f1 plane with hwdesc_f0. That's very confusing.

It is a tradeoff between code simplicity and confusion. Indeed difficult to decide. Fortunately I don't have to :)

> 
>> We have tested on jz4730 and jz4780.
> 
> Could I get a tested-by then? :)

I'll test with our tree and test both SoC in the next days and give feedback.

BR and thanks,
Nikolaus

> 
> Cheers,
> -Paul
> 
>> Maybe you want to consider that. Then I can officially post it.
>> [1] https://github.com/goldelico/letux-kernel/commit/3be1de5fdabf2cc1c17f198ded3328cc6e4b9844
>>> Am 24.01.2021 um 09:55 schrieb Paul Cercueil <paul@crapouillou.net>:
>>> Even though the JZ4740 did not have the OSD mode, it had (according to
>>> the documentation) two DMA channels, but there is absolutely no
>>> information about how to select the second DMA channel.
>>> Make the ingenic-drm driver work in non-OSD mode by using the
>>> foreground0 plane (which is bound to the DMA0 channel) as the primary
>>> plane, instead of the foreground1 plane, which is the primary plane
>>> when in OSD mode.
>>> Fixes: 3c9bea4ef32b ("drm/ingenic: Add support for OSD mode")
>>> Cc: <stable@vger.kernel.org> # v5.8+
>>> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
>>> Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>>> ---
>>> drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 11 +++++++----
>>> 1 file changed, 7 insertions(+), 4 deletions(-)
>>> diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
>>> index b23011c1c5d9..59ce43862e16 100644
>>> --- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
>>> +++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
>>> @@ -554,7 +554,7 @@ static void ingenic_drm_plane_atomic_update(struct drm_plane *plane,
>>> 		height = state->src_h >> 16;
>>> 		cpp = state->fb->format->cpp[0];
>>> -		if (priv->soc_info->has_osd && plane->type == DRM_PLANE_TYPE_OVERLAY)
>>> +		if (!priv->soc_info->has_osd || plane->type == DRM_PLANE_TYPE_OVERLAY)
>>> 			hwdesc = &priv->dma_hwdescs->hwdesc_f0;
>>> 		else
>>> 			hwdesc = &priv->dma_hwdescs->hwdesc_f1;
>> we just replace this with
>>                if (priv->soc_info->has_osd && plane->type != DRM_PLANE_TYPE_OVERLAY)
>>                        hwdesc = &priv->dma_hwdescs->hwdesc_f1;
>>                else
>>                        hwdesc = &priv->dma_hwdescs->hwdesc_f0;
>> and the remainder can stay as is.
>>> @@ -826,6 +826,7 @@ static int ingenic_drm_bind(struct device *dev, bool has_components)
>>> 	const struct jz_soc_info *soc_info;
>>> 	struct ingenic_drm *priv;
>>> 	struct clk *parent_clk;
>>> +	struct drm_plane *primary;
>>> 	struct drm_bridge *bridge;
>>> 	struct drm_panel *panel;
>>> 	struct drm_encoder *encoder;
>>> @@ -940,9 +941,11 @@ static int ingenic_drm_bind(struct device *dev, bool has_components)
>>> 	if (soc_info->has_osd)
>>> 		priv->ipu_plane = drm_plane_from_index(drm, 0);
>>> -	drm_plane_helper_add(&priv->f1, &ingenic_drm_plane_helper_funcs);
>>> +	primary = priv->soc_info->has_osd ? &priv->f1 : &priv->f0;
>>> -	ret = drm_universal_plane_init(drm, &priv->f1, 1,
>>> +	drm_plane_helper_add(primary, &ingenic_drm_plane_helper_funcs);
>>> +
>>> +	ret = drm_universal_plane_init(drm, primary, 1,
>>> 				       &ingenic_drm_primary_plane_funcs,
>>> 				       priv->soc_info->formats_f1,
>>> 				       priv->soc_info->num_formats_f1,
>>> @@ -954,7 +957,7 @@ static int ingenic_drm_bind(struct device *dev, bool has_components)
>>> 	drm_crtc_helper_add(&priv->crtc, &ingenic_drm_crtc_helper_funcs);
>>> -	ret = drm_crtc_init_with_planes(drm, &priv->crtc, &priv->f1,
>>> +	ret = drm_crtc_init_with_planes(drm, &priv->crtc, primary,
>>> 					NULL, &ingenic_drm_crtc_funcs, NULL);
>>> 	if (ret) {
>>> 		dev_err(dev, "Failed to init CRTC: %i\n", ret);
>>> --
>>> 2.29.2
>> BR and thanks,
>> Nikolaus
> 
> 

Re: [PATCH v3 4/4] drm/ingenic: Fix non-OSD mode

[H. Nikolaus Schaller]

Hi Paul,

> Am 24.01.2021 um 10:43 schrieb Paul Cercueil <paul@crapouillou.net>:
> 
> Hi Nikolaus,
> 
> Le dim. 24 janv. 2021 à 10:30, H. Nikolaus Schaller <hns@goldelico.com> a écrit :
>> Hi Paul,
>> we observed the same issue on the jz4730 (which is almost identical
>> to the jz4740 wrt. LCDC) and our solution [1] was simpler.
>> It leaves the hwdesc f0 and f1 as they are and just takes f1 for really
>> programming the first DMA descriptor if there is no OSD.
> 
> Disagreed. With your solution, it ends up using priv->f1 plane with hwdesc_f0. That's very confusing.

It is a tradeoff between code simplicity and confusion. Indeed difficult to decide. Fortunately I don't have to :)

> 
>> We have tested on jz4730 and jz4780.
> 
> Could I get a tested-by then? :)

I'll test with our tree and test both SoC in the next days and give feedback.

BR and thanks,
Nikolaus

> 
> Cheers,
> -Paul
> 
>> Maybe you want to consider that. Then I can officially post it.
>> [1] https://github.com/goldelico/letux-kernel/commit/3be1de5fdabf2cc1c17f198ded3328cc6e4b9844
>>> Am 24.01.2021 um 09:55 schrieb Paul Cercueil <paul@crapouillou.net>:
>>> Even though the JZ4740 did not have the OSD mode, it had (according to
>>> the documentation) two DMA channels, but there is absolutely no
>>> information about how to select the second DMA channel.
>>> Make the ingenic-drm driver work in non-OSD mode by using the
>>> foreground0 plane (which is bound to the DMA0 channel) as the primary
>>> plane, instead of the foreground1 plane, which is the primary plane
>>> when in OSD mode.
>>> Fixes: 3c9bea4ef32b ("drm/ingenic: Add support for OSD mode")
>>> Cc: <stable@vger.kernel.org> # v5.8+
>>> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
>>> Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>>> ---
>>> drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 11 +++++++----
>>> 1 file changed, 7 insertions(+), 4 deletions(-)
>>> diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
>>> index b23011c1c5d9..59ce43862e16 100644
>>> --- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
>>> +++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
>>> @@ -554,7 +554,7 @@ static void ingenic_drm_plane_atomic_update(struct drm_plane *plane,
>>> 		height = state->src_h >> 16;
>>> 		cpp = state->fb->format->cpp[0];
>>> -		if (priv->soc_info->has_osd && plane->type == DRM_PLANE_TYPE_OVERLAY)
>>> +		if (!priv->soc_info->has_osd || plane->type == DRM_PLANE_TYPE_OVERLAY)
>>> 			hwdesc = &priv->dma_hwdescs->hwdesc_f0;
>>> 		else
>>> 			hwdesc = &priv->dma_hwdescs->hwdesc_f1;
>> we just replace this with
>>                if (priv->soc_info->has_osd && plane->type != DRM_PLANE_TYPE_OVERLAY)
>>                        hwdesc = &priv->dma_hwdescs->hwdesc_f1;
>>                else
>>                        hwdesc = &priv->dma_hwdescs->hwdesc_f0;
>> and the remainder can stay as is.
>>> @@ -826,6 +826,7 @@ static int ingenic_drm_bind(struct device *dev, bool has_components)
>>> 	const struct jz_soc_info *soc_info;
>>> 	struct ingenic_drm *priv;
>>> 	struct clk *parent_clk;
>>> +	struct drm_plane *primary;
>>> 	struct drm_bridge *bridge;
>>> 	struct drm_panel *panel;
>>> 	struct drm_encoder *encoder;
>>> @@ -940,9 +941,11 @@ static int ingenic_drm_bind(struct device *dev, bool has_components)
>>> 	if (soc_info->has_osd)
>>> 		priv->ipu_plane = drm_plane_from_index(drm, 0);
>>> -	drm_plane_helper_add(&priv->f1, &ingenic_drm_plane_helper_funcs);
>>> +	primary = priv->soc_info->has_osd ? &priv->f1 : &priv->f0;
>>> -	ret = drm_universal_plane_init(drm, &priv->f1, 1,
>>> +	drm_plane_helper_add(primary, &ingenic_drm_plane_helper_funcs);
>>> +
>>> +	ret = drm_universal_plane_init(drm, primary, 1,
>>> 				       &ingenic_drm_primary_plane_funcs,
>>> 				       priv->soc_info->formats_f1,
>>> 				       priv->soc_info->num_formats_f1,
>>> @@ -954,7 +957,7 @@ static int ingenic_drm_bind(struct device *dev, bool has_components)
>>> 	drm_crtc_helper_add(&priv->crtc, &ingenic_drm_crtc_helper_funcs);
>>> -	ret = drm_crtc_init_with_planes(drm, &priv->crtc, &priv->f1,
>>> +	ret = drm_crtc_init_with_planes(drm, &priv->crtc, primary,
>>> 					NULL, &ingenic_drm_crtc_funcs, NULL);
>>> 	if (ret) {
>>> 		dev_err(dev, "Failed to init CRTC: %i\n", ret);
>>> --
>>> 2.29.2
>> BR and thanks,
>> Nikolaus
> 
> 

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 4/4] drm/ingenic: Fix non-OSD mode

[H. Nikolaus Schaller]

Hi Paul,

> Am 24.01.2021 um 10:47 schrieb H. Nikolaus Schaller <hns@goldelico.com>:
> 
> Hi Paul,
> 
>> Am 24.01.2021 um 10:43 schrieb Paul Cercueil <paul@crapouillou.net>:
>> 
>> Hi Nikolaus,
>> 
>> Le dim. 24 janv. 2021 à 10:30, H. Nikolaus Schaller <hns@goldelico.com> a écrit :
>>> Hi Paul,
>>> we observed the same issue on the jz4730 (which is almost identical
>>> to the jz4740 wrt. LCDC) and our solution [1] was simpler.
>>> It leaves the hwdesc f0 and f1 as they are and just takes f1 for really
>>> programming the first DMA descriptor if there is no OSD.
>> 
>> Disagreed. With your solution, it ends up using priv->f1 plane with hwdesc_f0. That's very confusing.
> 
> It is a tradeoff between code simplicity and confusion. Indeed difficult to decide. Fortunately I don't have to :)
> 
>> 
>>> We have tested on jz4730 and jz4780.
>> 
>> Could I get a tested-by then? :)
> 
> I'll test with our tree and test both SoC in the next days and give feedback.

Ok, works on both devices:

a) Skytone Alpha 400 compatible Letux 400 with not-yet-upstream jz4730 and internal panel
b) CI20 with external HDMI (tablet) and our not-yet-upstream HDMI code

So the hwdesc handling is as ok as it was with our patch. I'll keep a copy of yours in our
tree until it arrives upstream.

Tested-by: H. Nikolaus Schaller <hns@goldelico.com>

BR and thanks,
Nikolaus


> 
> BR and thanks,
> Nikolaus
> 
>> 
>> Cheers,
>> -Paul
>> 
>>> Maybe you want to consider that. Then I can officially post it.
>>> [1] https://github.com/goldelico/letux-kernel/commit/3be1de5fdabf2cc1c17f198ded3328cc6e4b9844
>>>> Am 24.01.2021 um 09:55 schrieb Paul Cercueil <paul@crapouillou.net>:
>>>> Even though the JZ4740 did not have the OSD mode, it had (according to
>>>> the documentation) two DMA channels, but there is absolutely no
>>>> information about how to select the second DMA channel.
>>>> Make the ingenic-drm driver work in non-OSD mode by using the
>>>> foreground0 plane (which is bound to the DMA0 channel) as the primary
>>>> plane, instead of the foreground1 plane, which is the primary plane
>>>> when in OSD mode.
>>>> Fixes: 3c9bea4ef32b ("drm/ingenic: Add support for OSD mode")
>>>> Cc: <stable@vger.kernel.org> # v5.8+
>>>> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
>>>> Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>>>> ---
>>>> drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 11 +++++++----
>>>> 1 file changed, 7 insertions(+), 4 deletions(-)
>>>> diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
>>>> index b23011c1c5d9..59ce43862e16 100644
>>>> --- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
>>>> +++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
>>>> @@ -554,7 +554,7 @@ static void ingenic_drm_plane_atomic_update(struct drm_plane *plane,
>>>> 		height = state->src_h >> 16;
>>>> 		cpp = state->fb->format->cpp[0];
>>>> -		if (priv->soc_info->has_osd && plane->type == DRM_PLANE_TYPE_OVERLAY)
>>>> +		if (!priv->soc_info->has_osd || plane->type == DRM_PLANE_TYPE_OVERLAY)
>>>> 			hwdesc = &priv->dma_hwdescs->hwdesc_f0;
>>>> 		else
>>>> 			hwdesc = &priv->dma_hwdescs->hwdesc_f1;
>>> we just replace this with
>>>               if (priv->soc_info->has_osd && plane->type != DRM_PLANE_TYPE_OVERLAY)
>>>                       hwdesc = &priv->dma_hwdescs->hwdesc_f1;
>>>               else
>>>                       hwdesc = &priv->dma_hwdescs->hwdesc_f0;
>>> and the remainder can stay as is.
>>>> @@ -826,6 +826,7 @@ static int ingenic_drm_bind(struct device *dev, bool has_components)
>>>> 	const struct jz_soc_info *soc_info;
>>>> 	struct ingenic_drm *priv;
>>>> 	struct clk *parent_clk;
>>>> +	struct drm_plane *primary;
>>>> 	struct drm_bridge *bridge;
>>>> 	struct drm_panel *panel;
>>>> 	struct drm_encoder *encoder;
>>>> @@ -940,9 +941,11 @@ static int ingenic_drm_bind(struct device *dev, bool has_components)
>>>> 	if (soc_info->has_osd)
>>>> 		priv->ipu_plane = drm_plane_from_index(drm, 0);
>>>> -	drm_plane_helper_add(&priv->f1, &ingenic_drm_plane_helper_funcs);
>>>> +	primary = priv->soc_info->has_osd ? &priv->f1 : &priv->f0;
>>>> -	ret = drm_universal_plane_init(drm, &priv->f1, 1,
>>>> +	drm_plane_helper_add(primary, &ingenic_drm_plane_helper_funcs);
>>>> +
>>>> +	ret = drm_universal_plane_init(drm, primary, 1,
>>>> 				       &ingenic_drm_primary_plane_funcs,
>>>> 				       priv->soc_info->formats_f1,
>>>> 				       priv->soc_info->num_formats_f1,
>>>> @@ -954,7 +957,7 @@ static int ingenic_drm_bind(struct device *dev, bool has_components)
>>>> 	drm_crtc_helper_add(&priv->crtc, &ingenic_drm_crtc_helper_funcs);
>>>> -	ret = drm_crtc_init_with_planes(drm, &priv->crtc, &priv->f1,
>>>> +	ret = drm_crtc_init_with_planes(drm, &priv->crtc, primary,
>>>> 					NULL, &ingenic_drm_crtc_funcs, NULL);
>>>> 	if (ret) {
>>>> 		dev_err(dev, "Failed to init CRTC: %i\n", ret);
>>>> --
>>>> 2.29.2
>>> BR and thanks,
>>> Nikolaus
>> 
>> 
> 

Re: [PATCH v3 4/4] drm/ingenic: Fix non-OSD mode

[H. Nikolaus Schaller]

Hi Paul,

> Am 24.01.2021 um 10:47 schrieb H. Nikolaus Schaller <hns@goldelico.com>:
> 
> Hi Paul,
> 
>> Am 24.01.2021 um 10:43 schrieb Paul Cercueil <paul@crapouillou.net>:
>> 
>> Hi Nikolaus,
>> 
>> Le dim. 24 janv. 2021 à 10:30, H. Nikolaus Schaller <hns@goldelico.com> a écrit :
>>> Hi Paul,
>>> we observed the same issue on the jz4730 (which is almost identical
>>> to the jz4740 wrt. LCDC) and our solution [1] was simpler.
>>> It leaves the hwdesc f0 and f1 as they are and just takes f1 for really
>>> programming the first DMA descriptor if there is no OSD.
>> 
>> Disagreed. With your solution, it ends up using priv->f1 plane with hwdesc_f0. That's very confusing.
> 
> It is a tradeoff between code simplicity and confusion. Indeed difficult to decide. Fortunately I don't have to :)
> 
>> 
>>> We have tested on jz4730 and jz4780.
>> 
>> Could I get a tested-by then? :)
> 
> I'll test with our tree and test both SoC in the next days and give feedback.

Ok, works on both devices:

a) Skytone Alpha 400 compatible Letux 400 with not-yet-upstream jz4730 and internal panel
b) CI20 with external HDMI (tablet) and our not-yet-upstream HDMI code

So the hwdesc handling is as ok as it was with our patch. I'll keep a copy of yours in our
tree until it arrives upstream.

Tested-by: H. Nikolaus Schaller <hns@goldelico.com>

BR and thanks,
Nikolaus


> 
> BR and thanks,
> Nikolaus
> 
>> 
>> Cheers,
>> -Paul
>> 
>>> Maybe you want to consider that. Then I can officially post it.
>>> [1] https://github.com/goldelico/letux-kernel/commit/3be1de5fdabf2cc1c17f198ded3328cc6e4b9844
>>>> Am 24.01.2021 um 09:55 schrieb Paul Cercueil <paul@crapouillou.net>:
>>>> Even though the JZ4740 did not have the OSD mode, it had (according to
>>>> the documentation) two DMA channels, but there is absolutely no
>>>> information about how to select the second DMA channel.
>>>> Make the ingenic-drm driver work in non-OSD mode by using the
>>>> foreground0 plane (which is bound to the DMA0 channel) as the primary
>>>> plane, instead of the foreground1 plane, which is the primary plane
>>>> when in OSD mode.
>>>> Fixes: 3c9bea4ef32b ("drm/ingenic: Add support for OSD mode")
>>>> Cc: <stable@vger.kernel.org> # v5.8+
>>>> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
>>>> Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>>>> ---
>>>> drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 11 +++++++----
>>>> 1 file changed, 7 insertions(+), 4 deletions(-)
>>>> diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
>>>> index b23011c1c5d9..59ce43862e16 100644
>>>> --- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
>>>> +++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
>>>> @@ -554,7 +554,7 @@ static void ingenic_drm_plane_atomic_update(struct drm_plane *plane,
>>>> 		height = state->src_h >> 16;
>>>> 		cpp = state->fb->format->cpp[0];
>>>> -		if (priv->soc_info->has_osd && plane->type == DRM_PLANE_TYPE_OVERLAY)
>>>> +		if (!priv->soc_info->has_osd || plane->type == DRM_PLANE_TYPE_OVERLAY)
>>>> 			hwdesc = &priv->dma_hwdescs->hwdesc_f0;
>>>> 		else
>>>> 			hwdesc = &priv->dma_hwdescs->hwdesc_f1;
>>> we just replace this with
>>>               if (priv->soc_info->has_osd && plane->type != DRM_PLANE_TYPE_OVERLAY)
>>>                       hwdesc = &priv->dma_hwdescs->hwdesc_f1;
>>>               else
>>>                       hwdesc = &priv->dma_hwdescs->hwdesc_f0;
>>> and the remainder can stay as is.
>>>> @@ -826,6 +826,7 @@ static int ingenic_drm_bind(struct device *dev, bool has_components)
>>>> 	const struct jz_soc_info *soc_info;
>>>> 	struct ingenic_drm *priv;
>>>> 	struct clk *parent_clk;
>>>> +	struct drm_plane *primary;
>>>> 	struct drm_bridge *bridge;
>>>> 	struct drm_panel *panel;
>>>> 	struct drm_encoder *encoder;
>>>> @@ -940,9 +941,11 @@ static int ingenic_drm_bind(struct device *dev, bool has_components)
>>>> 	if (soc_info->has_osd)
>>>> 		priv->ipu_plane = drm_plane_from_index(drm, 0);
>>>> -	drm_plane_helper_add(&priv->f1, &ingenic_drm_plane_helper_funcs);
>>>> +	primary = priv->soc_info->has_osd ? &priv->f1 : &priv->f0;
>>>> -	ret = drm_universal_plane_init(drm, &priv->f1, 1,
>>>> +	drm_plane_helper_add(primary, &ingenic_drm_plane_helper_funcs);
>>>> +
>>>> +	ret = drm_universal_plane_init(drm, primary, 1,
>>>> 				       &ingenic_drm_primary_plane_funcs,
>>>> 				       priv->soc_info->formats_f1,
>>>> 				       priv->soc_info->num_formats_f1,
>>>> @@ -954,7 +957,7 @@ static int ingenic_drm_bind(struct device *dev, bool has_components)
>>>> 	drm_crtc_helper_add(&priv->crtc, &ingenic_drm_crtc_helper_funcs);
>>>> -	ret = drm_crtc_init_with_planes(drm, &priv->crtc, &priv->f1,
>>>> +	ret = drm_crtc_init_with_planes(drm, &priv->crtc, primary,
>>>> 					NULL, &ingenic_drm_crtc_funcs, NULL);
>>>> 	if (ret) {
>>>> 		dev_err(dev, "Failed to init CRTC: %i\n", ret);
>>>> --
>>>> 2.29.2
>>> BR and thanks,
>>> Nikolaus
>> 
>> 
> 

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 0/4] Fixes to bridge/panel and ingenic-drm

[Paul Cercueil]

Hi,

Some feedback for patches 1-3? Laurent?

Cheers,
-Paul


Le dim. 24 janv. 2021 à 8:55, Paul Cercueil <paul@crapouillou.net> a 
écrit :
> Hi,
> 
> Here are three independent fixes. The first one addresses a
> use-after-free in bridge/panel.c; the second one addresses a
> use-after-free in the ingenic-drm driver; finally, the third one makes
> the ingenic-drm driver work again on older Ingenic SoCs.
> 
> Changes from v2:
> - patch [1/4] added a FIXME.
> - patch [2/4] is new. It introduces a 
> drmm_plain_simple_encoder_alloc()
>   macro that will be used in patch [3/4].
> - patch [3/4] uses the macro introduced in patch [2/4].
> - patch [4/4] is unmodified.
> 
> Note to linux-stable guys: patch [v2 2/3] will only apply on the 
> current
> drm-misc-next branch, to fix it for v5.11 and older kernels, use the 
> V1
> of that patch.
> 
> Cheers,
> -Paul
> 
> Paul Cercueil (4):
>   drm: bridge/panel: Cleanup connector on bridge detach
>   drm/simple_kms_helper: Add macro drmm_plain_simple_encoder_alloc()
>   drm/ingenic: Register devm action to cleanup encoders
>   drm/ingenic: Fix non-OSD mode
> 
>  drivers/gpu/drm/bridge/panel.c            | 12 +++++++++++
>  drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 26 
> +++++++++++------------
>  include/drm/drm_simple_kms_helper.h       | 17 +++++++++++++++
>  3 files changed, 42 insertions(+), 13 deletions(-)
> 
> --
> 2.29.2
> 

Re: [PATCH v3 0/4] Fixes to bridge/panel and ingenic-drm

[Paul Cercueil]

Hi,

Some feedback for patches 1-3? Laurent?

Cheers,
-Paul


Le dim. 24 janv. 2021 à 8:55, Paul Cercueil <paul@crapouillou.net> a 
écrit :
> Hi,
> 
> Here are three independent fixes. The first one addresses a
> use-after-free in bridge/panel.c; the second one addresses a
> use-after-free in the ingenic-drm driver; finally, the third one makes
> the ingenic-drm driver work again on older Ingenic SoCs.
> 
> Changes from v2:
> - patch [1/4] added a FIXME.
> - patch [2/4] is new. It introduces a 
> drmm_plain_simple_encoder_alloc()
>   macro that will be used in patch [3/4].
> - patch [3/4] uses the macro introduced in patch [2/4].
> - patch [4/4] is unmodified.
> 
> Note to linux-stable guys: patch [v2 2/3] will only apply on the 
> current
> drm-misc-next branch, to fix it for v5.11 and older kernels, use the 
> V1
> of that patch.
> 
> Cheers,
> -Paul
> 
> Paul Cercueil (4):
>   drm: bridge/panel: Cleanup connector on bridge detach
>   drm/simple_kms_helper: Add macro drmm_plain_simple_encoder_alloc()
>   drm/ingenic: Register devm action to cleanup encoders
>   drm/ingenic: Fix non-OSD mode
> 
>  drivers/gpu/drm/bridge/panel.c            | 12 +++++++++++
>  drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 26 
> +++++++++++------------
>  include/drm/drm_simple_kms_helper.h       | 17 +++++++++++++++
>  3 files changed, 42 insertions(+), 13 deletions(-)
> 
> --
> 2.29.2
> 


_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 0/4] Fixes to bridge/panel and ingenic-drm

[Paul Cercueil]

Le mer. 24 févr. 2021 à 13:44, Paul Cercueil <paul@crapouillou.net> a 
écrit :
> Hi,
> 
> Some feedback for patches 1-3? Laurent?

1-month anniversary ping :)

Cheers,
-Paul

> Cheers,
> -Paul
> 
> 
> Le dim. 24 janv. 2021 à 8:55, Paul Cercueil <paul@crapouillou.net> a 
> écrit :
>> Hi,
>> 
>> Here are three independent fixes. The first one addresses a
>> use-after-free in bridge/panel.c; the second one addresses a
>> use-after-free in the ingenic-drm driver; finally, the third one 
>> makes
>> the ingenic-drm driver work again on older Ingenic SoCs.
>> 
>> Changes from v2:
>> - patch [1/4] added a FIXME.
>> - patch [2/4] is new. It introduces a 
>> \x7fdrmm_plain_simple_encoder_alloc()
>>   macro that will be used in patch [3/4].
>> - patch [3/4] uses the macro introduced in patch [2/4].
>> - patch [4/4] is unmodified.
>> 
>> Note to linux-stable guys: patch [v2 2/3] will only apply on the 
>> \x7fcurrent
>> drm-misc-next branch, to fix it for v5.11 and older kernels, use the 
>> \x7fV1
>> of that patch.
>> 
>> Cheers,
>> -Paul
>> 
>> Paul Cercueil (4):
>>   drm: bridge/panel: Cleanup connector on bridge detach
>>   drm/simple_kms_helper: Add macro drmm_plain_simple_encoder_alloc()
>>   drm/ingenic: Register devm action to cleanup encoders
>>   drm/ingenic: Fix non-OSD mode
>> 
>>  drivers/gpu/drm/bridge/panel.c            | 12 +++++++++++
>>  drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 26 
>> \x7f+++++++++++------------
>>  include/drm/drm_simple_kms_helper.h       | 17 +++++++++++++++
>>  3 files changed, 42 insertions(+), 13 deletions(-)
>> 
>> --
>> 2.29.2
>> 
> 

Re: [PATCH v3 0/4] Fixes to bridge/panel and ingenic-drm

[Paul Cercueil]

Le mer. 24 févr. 2021 à 13:44, Paul Cercueil <paul@crapouillou.net> a 
écrit :
> Hi,
> 
> Some feedback for patches 1-3? Laurent?

1-month anniversary ping :)

Cheers,
-Paul

> Cheers,
> -Paul
> 
> 
> Le dim. 24 janv. 2021 à 8:55, Paul Cercueil <paul@crapouillou.net> a 
> écrit :
>> Hi,
>> 
>> Here are three independent fixes. The first one addresses a
>> use-after-free in bridge/panel.c; the second one addresses a
>> use-after-free in the ingenic-drm driver; finally, the third one 
>> makes
>> the ingenic-drm driver work again on older Ingenic SoCs.
>> 
>> Changes from v2:
>> - patch [1/4] added a FIXME.
>> - patch [2/4] is new. It introduces a 
>> \x7fdrmm_plain_simple_encoder_alloc()
>>   macro that will be used in patch [3/4].
>> - patch [3/4] uses the macro introduced in patch [2/4].
>> - patch [4/4] is unmodified.
>> 
>> Note to linux-stable guys: patch [v2 2/3] will only apply on the 
>> \x7fcurrent
>> drm-misc-next branch, to fix it for v5.11 and older kernels, use the 
>> \x7fV1
>> of that patch.
>> 
>> Cheers,
>> -Paul
>> 
>> Paul Cercueil (4):
>>   drm: bridge/panel: Cleanup connector on bridge detach
>>   drm/simple_kms_helper: Add macro drmm_plain_simple_encoder_alloc()
>>   drm/ingenic: Register devm action to cleanup encoders
>>   drm/ingenic: Fix non-OSD mode
>> 
>>  drivers/gpu/drm/bridge/panel.c            | 12 +++++++++++
>>  drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 26 
>> \x7f+++++++++++------------
>>  include/drm/drm_simple_kms_helper.h       | 17 +++++++++++++++
>>  3 files changed, 42 insertions(+), 13 deletions(-)
>> 
>> --
>> 2.29.2
>> 
> 


_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 1/4] drm: bridge/panel: Cleanup connector on bridge detach

[Laurent Pinchart]

Hi Paul,

Thank you for the patch.

On Sun, Jan 24, 2021 at 08:55:49AM +0000, Paul Cercueil wrote:
> If we don't call drm_connector_cleanup() manually in
> panel_bridge_detach(), the connector will be cleaned up with the other
> DRM objects in the call to drm_mode_config_cleanup(). However, since our
> drm_connector is devm-allocated, by the time drm_mode_config_cleanup()
> will be called, our connector will be long gone. Therefore, the
> connector must be cleaned up when the bridge is detached to avoid
> use-after-free conditions.
> 
> v2: Cleanup connector only if it was created
> 
> v3: Add FIXME
> 
> Fixes: 13dfc0540a57 ("drm/bridge: Refactor out the panel wrapper from the lvds-encoder bridge.")
> Cc: <stable@vger.kernel.org> # 4.12+
> Cc: Andrzej Hajda <a.hajda@samsung.com>
> Cc: Neil Armstrong <narmstrong@baylibre.com>
> Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
> Cc: Jonas Karlman <jonas@kwiboo.se>
> Cc: Jernej Skrabec <jernej.skrabec@siol.net>
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> ---
>  drivers/gpu/drm/bridge/panel.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c
> index 0ddc37551194..5959e8183cd0 100644
> --- a/drivers/gpu/drm/bridge/panel.c
> +++ b/drivers/gpu/drm/bridge/panel.c
> @@ -87,6 +87,18 @@ static int panel_bridge_attach(struct drm_bridge *bridge,
>  
>  static void panel_bridge_detach(struct drm_bridge *bridge)
>  {
> +	struct panel_bridge *panel_bridge = drm_bridge_to_panel_bridge(bridge);
> +	struct drm_connector *connector = &panel_bridge->connector;
> +
> +	/*
> +	 * Cleanup the connector if we know it was initialized.
> +	 *
> +	 * FIXME: This wouldn't be needed if the panel_bridge structure was
> +	 * allocated with drmm_kzalloc(). This might be tricky since the
> +	 * drm_device pointer can only be retrieved when the bridge is attached.
> +	 */
> +	if (!!panel_bridge->connector.dev)

How about simply

	if (connector->dev)

? With this change,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> +		drm_connector_cleanup(connector);
>  }
>  
>  static void panel_bridge_pre_enable(struct drm_bridge *bridge)

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 1/4] drm: bridge/panel: Cleanup connector on bridge detach

[Laurent Pinchart]

Hi Paul,

Thank you for the patch.

On Sun, Jan 24, 2021 at 08:55:49AM +0000, Paul Cercueil wrote:
> If we don't call drm_connector_cleanup() manually in
> panel_bridge_detach(), the connector will be cleaned up with the other
> DRM objects in the call to drm_mode_config_cleanup(). However, since our
> drm_connector is devm-allocated, by the time drm_mode_config_cleanup()
> will be called, our connector will be long gone. Therefore, the
> connector must be cleaned up when the bridge is detached to avoid
> use-after-free conditions.
> 
> v2: Cleanup connector only if it was created
> 
> v3: Add FIXME
> 
> Fixes: 13dfc0540a57 ("drm/bridge: Refactor out the panel wrapper from the lvds-encoder bridge.")
> Cc: <stable@vger.kernel.org> # 4.12+
> Cc: Andrzej Hajda <a.hajda@samsung.com>
> Cc: Neil Armstrong <narmstrong@baylibre.com>
> Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
> Cc: Jonas Karlman <jonas@kwiboo.se>
> Cc: Jernej Skrabec <jernej.skrabec@siol.net>
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> ---
>  drivers/gpu/drm/bridge/panel.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c
> index 0ddc37551194..5959e8183cd0 100644
> --- a/drivers/gpu/drm/bridge/panel.c
> +++ b/drivers/gpu/drm/bridge/panel.c
> @@ -87,6 +87,18 @@ static int panel_bridge_attach(struct drm_bridge *bridge,
>  
>  static void panel_bridge_detach(struct drm_bridge *bridge)
>  {
> +	struct panel_bridge *panel_bridge = drm_bridge_to_panel_bridge(bridge);
> +	struct drm_connector *connector = &panel_bridge->connector;
> +
> +	/*
> +	 * Cleanup the connector if we know it was initialized.
> +	 *
> +	 * FIXME: This wouldn't be needed if the panel_bridge structure was
> +	 * allocated with drmm_kzalloc(). This might be tricky since the
> +	 * drm_device pointer can only be retrieved when the bridge is attached.
> +	 */
> +	if (!!panel_bridge->connector.dev)

How about simply

	if (connector->dev)

? With this change,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> +		drm_connector_cleanup(connector);
>  }
>  
>  static void panel_bridge_pre_enable(struct drm_bridge *bridge)

-- 
Regards,

Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 2/4] drm/simple_kms_helper: Add macro drmm_plain_simple_encoder_alloc()

[Laurent Pinchart]

Hi Paul,

Thank you for the patch.

On Sun, Jan 24, 2021 at 08:55:50AM +0000, Paul Cercueil wrote:
> This performs the same operation as drmm_simple_encoder_alloc(), but
> only allocates and returns a struct drm_encoder instance.
> 
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> ---
>  include/drm/drm_simple_kms_helper.h | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/include/drm/drm_simple_kms_helper.h b/include/drm/drm_simple_kms_helper.h
> index e6dbf3161c2f..f07e70303cfb 100644
> --- a/include/drm/drm_simple_kms_helper.h
> +++ b/include/drm/drm_simple_kms_helper.h
> @@ -209,4 +209,21 @@ void *__drmm_simple_encoder_alloc(struct drm_device *dev, size_t size,
>  					     offsetof(type, member), \
>  					     encoder_type))
>  
> +/**
> + * drmm_plain_simple_encoder_alloc - Allocate and initialize a drm_encoder
> + *                                   struct with basic functionality.
> + * @dev: drm device
> + * @encoder_type: user visible type of the encoder
> + *
> + * This performs the same operation as drmm_simple_encoder_alloc(), but
> + * only allocates and returns a struct drm_encoder instance.
> + *
> + * Returns:
> + * Pointer to the new drm_encoder struct, or ERR_PTR on failure.
> + */
> +#define drmm_plain_simple_encoder_alloc(dev, encoder_type) \
> +	((struct drm_encoder *) \
> +	 __drmm_simple_encoder_alloc(dev, sizeof(struct drm_encoder), \
> +				     0, encoder_type))
> +

As this isn't related to the simple encoder helper anymore, how about
using __drmm_encoder_alloc instead ?

#define drmm_plain_simple_encoder_alloc(dev, encoder_type) \
	((struct drm_encoder *) \
	__drmm_encoder_alloc(dev, sizeof(struct drm_encoder), 0, NULL, \
			     encoder_type, NULL))

I'd also rename the macro to drmm_plain_encoder_alloc(), and move it to
drm_encoder.h. That way drivers that don't need the simple KMS helper
won't have to select it just for this.

>  #endif /* __LINUX_DRM_SIMPLE_KMS_HELPER_H */

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 2/4] drm/simple_kms_helper: Add macro drmm_plain_simple_encoder_alloc()

[Laurent Pinchart]

Hi Paul,

Thank you for the patch.

On Sun, Jan 24, 2021 at 08:55:50AM +0000, Paul Cercueil wrote:
> This performs the same operation as drmm_simple_encoder_alloc(), but
> only allocates and returns a struct drm_encoder instance.
> 
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> ---
>  include/drm/drm_simple_kms_helper.h | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/include/drm/drm_simple_kms_helper.h b/include/drm/drm_simple_kms_helper.h
> index e6dbf3161c2f..f07e70303cfb 100644
> --- a/include/drm/drm_simple_kms_helper.h
> +++ b/include/drm/drm_simple_kms_helper.h
> @@ -209,4 +209,21 @@ void *__drmm_simple_encoder_alloc(struct drm_device *dev, size_t size,
>  					     offsetof(type, member), \
>  					     encoder_type))
>  
> +/**
> + * drmm_plain_simple_encoder_alloc - Allocate and initialize a drm_encoder
> + *                                   struct with basic functionality.
> + * @dev: drm device
> + * @encoder_type: user visible type of the encoder
> + *
> + * This performs the same operation as drmm_simple_encoder_alloc(), but
> + * only allocates and returns a struct drm_encoder instance.
> + *
> + * Returns:
> + * Pointer to the new drm_encoder struct, or ERR_PTR on failure.
> + */
> +#define drmm_plain_simple_encoder_alloc(dev, encoder_type) \
> +	((struct drm_encoder *) \
> +	 __drmm_simple_encoder_alloc(dev, sizeof(struct drm_encoder), \
> +				     0, encoder_type))
> +

As this isn't related to the simple encoder helper anymore, how about
using __drmm_encoder_alloc instead ?

#define drmm_plain_simple_encoder_alloc(dev, encoder_type) \
	((struct drm_encoder *) \
	__drmm_encoder_alloc(dev, sizeof(struct drm_encoder), 0, NULL, \
			     encoder_type, NULL))

I'd also rename the macro to drmm_plain_encoder_alloc(), and move it to
drm_encoder.h. That way drivers that don't need the simple KMS helper
won't have to select it just for this.

>  #endif /* __LINUX_DRM_SIMPLE_KMS_HELPER_H */

-- 
Regards,

Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 3/4] drm/ingenic: Register devm action to cleanup encoders

[Laurent Pinchart]

Hi Paul,

Thank you for the patch.

On Sun, Jan 24, 2021 at 08:55:51AM +0000, Paul Cercueil wrote:
> Since the encoders have been devm-allocated, they will be freed way
> before drm_mode_config_cleanup() is called. To avoid use-after-free
> conditions, we then must ensure that drm_encoder_cleanup() is called
> before the encoders are freed.
> 
> v2: Use the new __drmm_simple_encoder_alloc() function
> 
> v3: Use the new drmm_plain_simple_encoder_alloc() macro
> 
> Fixes: c369cb27c267 ("drm/ingenic: Support multiple panels/bridges")
> Cc: <stable@vger.kernel.org> # 5.8+
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
> 
> Notes:
>     Use the V1 of this patch to fix v5.11 and older kernels. This V3 only
>     applies on the current drm-misc-next branch.
> 
>  drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 15 ++++++---------
>  1 file changed, 6 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> index 7bb31fbee29d..b23011c1c5d9 100644
> --- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> +++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> @@ -1014,20 +1014,17 @@ static int ingenic_drm_bind(struct device *dev, bool has_components)
>  			bridge = devm_drm_panel_bridge_add_typed(dev, panel,
>  								 DRM_MODE_CONNECTOR_DPI);
>  
> -		encoder = devm_kzalloc(dev, sizeof(*encoder), GFP_KERNEL);
> -		if (!encoder)
> -			return -ENOMEM;
> +		encoder = drmm_plain_simple_encoder_alloc(drm, DRM_MODE_ENCODER_DPI);
> +		if (IS_ERR(encoder)) {
> +			ret = PTR_ERR(encoder);
> +			dev_err(dev, "Failed to init encoder: %d\n", ret);
> +			return ret;
> +		}
>  
>  		encoder->possible_crtcs = 1;
>  
>  		drm_encoder_helper_add(encoder, &ingenic_drm_encoder_helper_funcs);
>  
> -		ret = drm_simple_encoder_init(drm, encoder, DRM_MODE_ENCODER_DPI);
> -		if (ret) {
> -			dev_err(dev, "Failed to init encoder: %d\n", ret);
> -			return ret;
> -		}
> -
>  		ret = drm_bridge_attach(encoder, bridge, NULL, 0);
>  		if (ret) {
>  			dev_err(dev, "Unable to attach bridge\n");

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 3/4] drm/ingenic: Register devm action to cleanup encoders

[Laurent Pinchart]

Hi Paul,

Thank you for the patch.

On Sun, Jan 24, 2021 at 08:55:51AM +0000, Paul Cercueil wrote:
> Since the encoders have been devm-allocated, they will be freed way
> before drm_mode_config_cleanup() is called. To avoid use-after-free
> conditions, we then must ensure that drm_encoder_cleanup() is called
> before the encoders are freed.
> 
> v2: Use the new __drmm_simple_encoder_alloc() function
> 
> v3: Use the new drmm_plain_simple_encoder_alloc() macro
> 
> Fixes: c369cb27c267 ("drm/ingenic: Support multiple panels/bridges")
> Cc: <stable@vger.kernel.org> # 5.8+
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
> 
> Notes:
>     Use the V1 of this patch to fix v5.11 and older kernels. This V3 only
>     applies on the current drm-misc-next branch.
> 
>  drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 15 ++++++---------
>  1 file changed, 6 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> index 7bb31fbee29d..b23011c1c5d9 100644
> --- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> +++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> @@ -1014,20 +1014,17 @@ static int ingenic_drm_bind(struct device *dev, bool has_components)
>  			bridge = devm_drm_panel_bridge_add_typed(dev, panel,
>  								 DRM_MODE_CONNECTOR_DPI);
>  
> -		encoder = devm_kzalloc(dev, sizeof(*encoder), GFP_KERNEL);
> -		if (!encoder)
> -			return -ENOMEM;
> +		encoder = drmm_plain_simple_encoder_alloc(drm, DRM_MODE_ENCODER_DPI);
> +		if (IS_ERR(encoder)) {
> +			ret = PTR_ERR(encoder);
> +			dev_err(dev, "Failed to init encoder: %d\n", ret);
> +			return ret;
> +		}
>  
>  		encoder->possible_crtcs = 1;
>  
>  		drm_encoder_helper_add(encoder, &ingenic_drm_encoder_helper_funcs);
>  
> -		ret = drm_simple_encoder_init(drm, encoder, DRM_MODE_ENCODER_DPI);
> -		if (ret) {
> -			dev_err(dev, "Failed to init encoder: %d\n", ret);
> -			return ret;
> -		}
> -
>  		ret = drm_bridge_attach(encoder, bridge, NULL, 0);
>  		if (ret) {
>  			dev_err(dev, "Unable to attach bridge\n");

-- 
Regards,

Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 4/4] drm/ingenic: Fix non-OSD mode

[Laurent Pinchart]

Hi Paul,

Thank you for the patch.

On Sun, Jan 24, 2021 at 08:55:52AM +0000, Paul Cercueil wrote:
> Even though the JZ4740 did not have the OSD mode, it had (according to
> the documentation) two DMA channels, but there is absolutely no
> information about how to select the second DMA channel.
> 
> Make the ingenic-drm driver work in non-OSD mode by using the
> foreground0 plane (which is bound to the DMA0 channel) as the primary
> plane, instead of the foreground1 plane, which is the primary plane
> when in OSD mode.
> 
> Fixes: 3c9bea4ef32b ("drm/ingenic: Add support for OSD mode")
> Cc: <stable@vger.kernel.org> # v5.8+
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>

I don't have much knowledge about this platform, but the change looks
reasonable to me.

Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> index b23011c1c5d9..59ce43862e16 100644
> --- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> +++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> @@ -554,7 +554,7 @@ static void ingenic_drm_plane_atomic_update(struct drm_plane *plane,
>  		height = state->src_h >> 16;
>  		cpp = state->fb->format->cpp[0];
>  
> -		if (priv->soc_info->has_osd && plane->type == DRM_PLANE_TYPE_OVERLAY)
> +		if (!priv->soc_info->has_osd || plane->type == DRM_PLANE_TYPE_OVERLAY)
>  			hwdesc = &priv->dma_hwdescs->hwdesc_f0;
>  		else
>  			hwdesc = &priv->dma_hwdescs->hwdesc_f1;
> @@ -826,6 +826,7 @@ static int ingenic_drm_bind(struct device *dev, bool has_components)
>  	const struct jz_soc_info *soc_info;
>  	struct ingenic_drm *priv;
>  	struct clk *parent_clk;
> +	struct drm_plane *primary;
>  	struct drm_bridge *bridge;
>  	struct drm_panel *panel;
>  	struct drm_encoder *encoder;
> @@ -940,9 +941,11 @@ static int ingenic_drm_bind(struct device *dev, bool has_components)
>  	if (soc_info->has_osd)
>  		priv->ipu_plane = drm_plane_from_index(drm, 0);
>  
> -	drm_plane_helper_add(&priv->f1, &ingenic_drm_plane_helper_funcs);
> +	primary = priv->soc_info->has_osd ? &priv->f1 : &priv->f0;
>  
> -	ret = drm_universal_plane_init(drm, &priv->f1, 1,
> +	drm_plane_helper_add(primary, &ingenic_drm_plane_helper_funcs);
> +
> +	ret = drm_universal_plane_init(drm, primary, 1,
>  				       &ingenic_drm_primary_plane_funcs,
>  				       priv->soc_info->formats_f1,
>  				       priv->soc_info->num_formats_f1,
> @@ -954,7 +957,7 @@ static int ingenic_drm_bind(struct device *dev, bool has_components)
>  
>  	drm_crtc_helper_add(&priv->crtc, &ingenic_drm_crtc_helper_funcs);
>  
> -	ret = drm_crtc_init_with_planes(drm, &priv->crtc, &priv->f1,
> +	ret = drm_crtc_init_with_planes(drm, &priv->crtc, primary,
>  					NULL, &ingenic_drm_crtc_funcs, NULL);
>  	if (ret) {
>  		dev_err(dev, "Failed to init CRTC: %i\n", ret);

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 4/4] drm/ingenic: Fix non-OSD mode

[Laurent Pinchart]

Hi Paul,

Thank you for the patch.

On Sun, Jan 24, 2021 at 08:55:52AM +0000, Paul Cercueil wrote:
> Even though the JZ4740 did not have the OSD mode, it had (according to
> the documentation) two DMA channels, but there is absolutely no
> information about how to select the second DMA channel.
> 
> Make the ingenic-drm driver work in non-OSD mode by using the
> foreground0 plane (which is bound to the DMA0 channel) as the primary
> plane, instead of the foreground1 plane, which is the primary plane
> when in OSD mode.
> 
> Fixes: 3c9bea4ef32b ("drm/ingenic: Add support for OSD mode")
> Cc: <stable@vger.kernel.org> # v5.8+
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>

I don't have much knowledge about this platform, but the change looks
reasonable to me.

Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> index b23011c1c5d9..59ce43862e16 100644
> --- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> +++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> @@ -554,7 +554,7 @@ static void ingenic_drm_plane_atomic_update(struct drm_plane *plane,
>  		height = state->src_h >> 16;
>  		cpp = state->fb->format->cpp[0];
>  
> -		if (priv->soc_info->has_osd && plane->type == DRM_PLANE_TYPE_OVERLAY)
> +		if (!priv->soc_info->has_osd || plane->type == DRM_PLANE_TYPE_OVERLAY)
>  			hwdesc = &priv->dma_hwdescs->hwdesc_f0;
>  		else
>  			hwdesc = &priv->dma_hwdescs->hwdesc_f1;
> @@ -826,6 +826,7 @@ static int ingenic_drm_bind(struct device *dev, bool has_components)
>  	const struct jz_soc_info *soc_info;
>  	struct ingenic_drm *priv;
>  	struct clk *parent_clk;
> +	struct drm_plane *primary;
>  	struct drm_bridge *bridge;
>  	struct drm_panel *panel;
>  	struct drm_encoder *encoder;
> @@ -940,9 +941,11 @@ static int ingenic_drm_bind(struct device *dev, bool has_components)
>  	if (soc_info->has_osd)
>  		priv->ipu_plane = drm_plane_from_index(drm, 0);
>  
> -	drm_plane_helper_add(&priv->f1, &ingenic_drm_plane_helper_funcs);
> +	primary = priv->soc_info->has_osd ? &priv->f1 : &priv->f0;
>  
> -	ret = drm_universal_plane_init(drm, &priv->f1, 1,
> +	drm_plane_helper_add(primary, &ingenic_drm_plane_helper_funcs);
> +
> +	ret = drm_universal_plane_init(drm, primary, 1,
>  				       &ingenic_drm_primary_plane_funcs,
>  				       priv->soc_info->formats_f1,
>  				       priv->soc_info->num_formats_f1,
> @@ -954,7 +957,7 @@ static int ingenic_drm_bind(struct device *dev, bool has_components)
>  
>  	drm_crtc_helper_add(&priv->crtc, &ingenic_drm_crtc_helper_funcs);
>  
> -	ret = drm_crtc_init_with_planes(drm, &priv->crtc, &priv->f1,
> +	ret = drm_crtc_init_with_planes(drm, &priv->crtc, primary,
>  					NULL, &ingenic_drm_crtc_funcs, NULL);
>  	if (ret) {
>  		dev_err(dev, "Failed to init CRTC: %i\n", ret);

-- 
Regards,

Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 0/4] Fixes to bridge/panel and ingenic-drm

[Laurent Pinchart]

On Tue, Mar 23, 2021 at 04:03:00PM +0000, Paul Cercueil wrote:
> Le mer. 24 févr. 2021 à 13:44, Paul Cercueil a écrit :
> > Hi,
> > 
> > Some feedback for patches 1-3? Laurent?
> 
> 1-month anniversary ping :)

Haaappy birth-day toooo youuuuuuu :-)

Patches reviewed.

> > Le dim. 24 janv. 2021 à 8:55, Paul Cercueil a  écrit :
> >> Hi,
> >> 
> >> Here are three independent fixes. The first one addresses a
> >> use-after-free in bridge/panel.c; the second one addresses a
> >> use-after-free in the ingenic-drm driver; finally, the third one makes
> >> the ingenic-drm driver work again on older Ingenic SoCs.
> >> 
> >> Changes from v2:
> >> - patch [1/4] added a FIXME.
> >> - patch [2/4] is new. It introduces a drmm_plain_simple_encoder_alloc()
> >>   macro that will be used in patch [3/4].
> >> - patch [3/4] uses the macro introduced in patch [2/4].
> >> - patch [4/4] is unmodified.
> >> 
> >> Note to linux-stable guys: patch [v2 2/3] will only apply on the current
> >> drm-misc-next branch, to fix it for v5.11 and older kernels, use the V1
> >> of that patch.
> >> 
> >> Cheers,
> >> -Paul
> >> 
> >> Paul Cercueil (4):
> >>   drm: bridge/panel: Cleanup connector on bridge detach
> >>   drm/simple_kms_helper: Add macro drmm_plain_simple_encoder_alloc()
> >>   drm/ingenic: Register devm action to cleanup encoders
> >>   drm/ingenic: Fix non-OSD mode
> >> 
> >>  drivers/gpu/drm/bridge/panel.c            | 12 +++++++++++
> >>  drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 26 +++++++++++------------
> >>  include/drm/drm_simple_kms_helper.h       | 17 +++++++++++++++
> >>  3 files changed, 42 insertions(+), 13 deletions(-)
> >> 

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 0/4] Fixes to bridge/panel and ingenic-drm

[Laurent Pinchart]

On Tue, Mar 23, 2021 at 04:03:00PM +0000, Paul Cercueil wrote:
> Le mer. 24 févr. 2021 à 13:44, Paul Cercueil a écrit :
> > Hi,
> > 
> > Some feedback for patches 1-3? Laurent?
> 
> 1-month anniversary ping :)

Haaappy birth-day toooo youuuuuuu :-)

Patches reviewed.

> > Le dim. 24 janv. 2021 à 8:55, Paul Cercueil a  écrit :
> >> Hi,
> >> 
> >> Here are three independent fixes. The first one addresses a
> >> use-after-free in bridge/panel.c; the second one addresses a
> >> use-after-free in the ingenic-drm driver; finally, the third one makes
> >> the ingenic-drm driver work again on older Ingenic SoCs.
> >> 
> >> Changes from v2:
> >> - patch [1/4] added a FIXME.
> >> - patch [2/4] is new. It introduces a drmm_plain_simple_encoder_alloc()
> >>   macro that will be used in patch [3/4].
> >> - patch [3/4] uses the macro introduced in patch [2/4].
> >> - patch [4/4] is unmodified.
> >> 
> >> Note to linux-stable guys: patch [v2 2/3] will only apply on the current
> >> drm-misc-next branch, to fix it for v5.11 and older kernels, use the V1
> >> of that patch.
> >> 
> >> Cheers,
> >> -Paul
> >> 
> >> Paul Cercueil (4):
> >>   drm: bridge/panel: Cleanup connector on bridge detach
> >>   drm/simple_kms_helper: Add macro drmm_plain_simple_encoder_alloc()
> >>   drm/ingenic: Register devm action to cleanup encoders
> >>   drm/ingenic: Fix non-OSD mode
> >> 
> >>  drivers/gpu/drm/bridge/panel.c            | 12 +++++++++++
> >>  drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 26 +++++++++++------------
> >>  include/drm/drm_simple_kms_helper.h       | 17 +++++++++++++++
> >>  3 files changed, 42 insertions(+), 13 deletions(-)
> >> 

-- 
Regards,

Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel