Hi Neil,
this series consists of a small cleanup patch and a fix for the error handling in meson_drv_bind_master() when AFBCD is used. The patches are based on drm-misc to not conflict with your previous driver rework. Since the problem has not been observed in the wild I decided not to Cc linux-stable.
Best regards, Martin
Martin Blumenstingl (2): drm/meson: osd_afbcd: Add an exit callback to struct meson_afbcd_ops drm/meson: Fix error handling when afbcd.ops->init fails
drivers/gpu/drm/meson/meson_drv.c | 25 +++++++-------- drivers/gpu/drm/meson/meson_osd_afbcd.c | 41 ++++++++++++++++--------- drivers/gpu/drm/meson/meson_osd_afbcd.h | 1 + 3 files changed, 41 insertions(+), 26 deletions(-)
Use this to simplify the driver shutdown. It will also come handy when fixing the error handling in meson_drv_bind_master().
Signed-off-by: Martin Blumenstingl martin.blumenstingl@googlemail.com --- drivers/gpu/drm/meson/meson_drv.c | 6 ++-- drivers/gpu/drm/meson/meson_osd_afbcd.c | 41 ++++++++++++++++--------- drivers/gpu/drm/meson/meson_osd_afbcd.h | 1 + 3 files changed, 30 insertions(+), 18 deletions(-)
diff --git a/drivers/gpu/drm/meson/meson_drv.c b/drivers/gpu/drm/meson/meson_drv.c index 80f1d439841a..b919271a6e50 100644 --- a/drivers/gpu/drm/meson/meson_drv.c +++ b/drivers/gpu/drm/meson/meson_drv.c @@ -385,10 +385,8 @@ static void meson_drv_unbind(struct device *dev) free_irq(priv->vsync_irq, drm); drm_dev_put(drm);
- if (priv->afbcd.ops) { - priv->afbcd.ops->reset(priv); - meson_rdma_free(priv); - } + if (priv->afbcd.ops) + priv->afbcd.ops->exit(priv); }
static const struct component_master_ops meson_drv_master_ops = { diff --git a/drivers/gpu/drm/meson/meson_osd_afbcd.c b/drivers/gpu/drm/meson/meson_osd_afbcd.c index ffc6b584dbf8..0cdbe899402f 100644 --- a/drivers/gpu/drm/meson/meson_osd_afbcd.c +++ b/drivers/gpu/drm/meson/meson_osd_afbcd.c @@ -79,11 +79,6 @@ static bool meson_gxm_afbcd_supported_fmt(u64 modifier, uint32_t format) return meson_gxm_afbcd_pixel_fmt(modifier, format) >= 0; }
-static int meson_gxm_afbcd_init(struct meson_drm *priv) -{ - return 0; -} - static int meson_gxm_afbcd_reset(struct meson_drm *priv) { writel_relaxed(VIU_SW_RESET_OSD1_AFBCD, @@ -93,6 +88,16 @@ static int meson_gxm_afbcd_reset(struct meson_drm *priv) return 0; }
+static int meson_gxm_afbcd_init(struct meson_drm *priv) +{ + return 0; +} + +static void meson_gxm_afbcd_exit(struct meson_drm *priv) +{ + meson_gxm_afbcd_reset(priv); +} + static int meson_gxm_afbcd_enable(struct meson_drm *priv) { writel_relaxed(FIELD_PREP(OSD1_AFBCD_ID_FIFO_THRD, 0x40) | @@ -172,6 +177,7 @@ static int meson_gxm_afbcd_setup(struct meson_drm *priv)
struct meson_afbcd_ops meson_afbcd_gxm_ops = { .init = meson_gxm_afbcd_init, + .exit = meson_gxm_afbcd_exit, .reset = meson_gxm_afbcd_reset, .enable = meson_gxm_afbcd_enable, .disable = meson_gxm_afbcd_disable, @@ -269,6 +275,18 @@ static bool meson_g12a_afbcd_supported_fmt(u64 modifier, uint32_t format) return meson_g12a_afbcd_pixel_fmt(modifier, format) >= 0; }
+static int meson_g12a_afbcd_reset(struct meson_drm *priv) +{ + meson_rdma_reset(priv); + + meson_rdma_writel_sync(priv, VIU_SW_RESET_G12A_AFBC_ARB | + VIU_SW_RESET_G12A_OSD1_AFBCD, + VIU_SW_RESET); + meson_rdma_writel_sync(priv, 0, VIU_SW_RESET); + + return 0; +} + static int meson_g12a_afbcd_init(struct meson_drm *priv) { int ret; @@ -286,16 +304,10 @@ static int meson_g12a_afbcd_init(struct meson_drm *priv) return 0; }
-static int meson_g12a_afbcd_reset(struct meson_drm *priv) +static void meson_g12a_afbcd_exit(struct meson_drm *priv) { - meson_rdma_reset(priv); - - meson_rdma_writel_sync(priv, VIU_SW_RESET_G12A_AFBC_ARB | - VIU_SW_RESET_G12A_OSD1_AFBCD, - VIU_SW_RESET); - meson_rdma_writel_sync(priv, 0, VIU_SW_RESET); - - return 0; + meson_g12a_afbcd_reset(priv); + meson_rdma_free(priv); }
static int meson_g12a_afbcd_enable(struct meson_drm *priv) @@ -380,6 +392,7 @@ static int meson_g12a_afbcd_setup(struct meson_drm *priv)
struct meson_afbcd_ops meson_afbcd_g12a_ops = { .init = meson_g12a_afbcd_init, + .exit = meson_g12a_afbcd_exit, .reset = meson_g12a_afbcd_reset, .enable = meson_g12a_afbcd_enable, .disable = meson_g12a_afbcd_disable, diff --git a/drivers/gpu/drm/meson/meson_osd_afbcd.h b/drivers/gpu/drm/meson/meson_osd_afbcd.h index 5e5523304f42..e77ddeb6416f 100644 --- a/drivers/gpu/drm/meson/meson_osd_afbcd.h +++ b/drivers/gpu/drm/meson/meson_osd_afbcd.h @@ -14,6 +14,7 @@
struct meson_afbcd_ops { int (*init)(struct meson_drm *priv); + void (*exit)(struct meson_drm *priv); int (*reset)(struct meson_drm *priv); int (*enable)(struct meson_drm *priv); int (*disable)(struct meson_drm *priv);
On 31/12/2021 00:55, Martin Blumenstingl wrote:
Use this to simplify the driver shutdown. It will also come handy when fixing the error handling in meson_drv_bind_master().
Signed-off-by: Martin Blumenstingl martin.blumenstingl@googlemail.com
drivers/gpu/drm/meson/meson_drv.c | 6 ++-- drivers/gpu/drm/meson/meson_osd_afbcd.c | 41 ++++++++++++++++--------- drivers/gpu/drm/meson/meson_osd_afbcd.h | 1 + 3 files changed, 30 insertions(+), 18 deletions(-)
diff --git a/drivers/gpu/drm/meson/meson_drv.c b/drivers/gpu/drm/meson/meson_drv.c index 80f1d439841a..b919271a6e50 100644 --- a/drivers/gpu/drm/meson/meson_drv.c +++ b/drivers/gpu/drm/meson/meson_drv.c @@ -385,10 +385,8 @@ static void meson_drv_unbind(struct device *dev) free_irq(priv->vsync_irq, drm); drm_dev_put(drm);
- if (priv->afbcd.ops) {
priv->afbcd.ops->reset(priv);meson_rdma_free(priv);- }
- if (priv->afbcd.ops)
priv->afbcd.ops->exit(priv);}
static const struct component_master_ops meson_drv_master_ops = { diff --git a/drivers/gpu/drm/meson/meson_osd_afbcd.c b/drivers/gpu/drm/meson/meson_osd_afbcd.c index ffc6b584dbf8..0cdbe899402f 100644 --- a/drivers/gpu/drm/meson/meson_osd_afbcd.c +++ b/drivers/gpu/drm/meson/meson_osd_afbcd.c @@ -79,11 +79,6 @@ static bool meson_gxm_afbcd_supported_fmt(u64 modifier, uint32_t format) return meson_gxm_afbcd_pixel_fmt(modifier, format) >= 0; }
-static int meson_gxm_afbcd_init(struct meson_drm *priv) -{
- return 0;
-}
static int meson_gxm_afbcd_reset(struct meson_drm *priv) { writel_relaxed(VIU_SW_RESET_OSD1_AFBCD, @@ -93,6 +88,16 @@ static int meson_gxm_afbcd_reset(struct meson_drm *priv) return 0; }
+static int meson_gxm_afbcd_init(struct meson_drm *priv) +{
- return 0;
+}
+static void meson_gxm_afbcd_exit(struct meson_drm *priv) +{
- meson_gxm_afbcd_reset(priv);
+}
static int meson_gxm_afbcd_enable(struct meson_drm *priv) { writel_relaxed(FIELD_PREP(OSD1_AFBCD_ID_FIFO_THRD, 0x40) | @@ -172,6 +177,7 @@ static int meson_gxm_afbcd_setup(struct meson_drm *priv)
struct meson_afbcd_ops meson_afbcd_gxm_ops = { .init = meson_gxm_afbcd_init,
- .exit = meson_gxm_afbcd_exit, .reset = meson_gxm_afbcd_reset, .enable = meson_gxm_afbcd_enable, .disable = meson_gxm_afbcd_disable,
@@ -269,6 +275,18 @@ static bool meson_g12a_afbcd_supported_fmt(u64 modifier, uint32_t format) return meson_g12a_afbcd_pixel_fmt(modifier, format) >= 0; }
+static int meson_g12a_afbcd_reset(struct meson_drm *priv) +{
- meson_rdma_reset(priv);
- meson_rdma_writel_sync(priv, VIU_SW_RESET_G12A_AFBC_ARB |
VIU_SW_RESET_G12A_OSD1_AFBCD,VIU_SW_RESET);- meson_rdma_writel_sync(priv, 0, VIU_SW_RESET);
- return 0;
+}
static int meson_g12a_afbcd_init(struct meson_drm *priv) { int ret; @@ -286,16 +304,10 @@ static int meson_g12a_afbcd_init(struct meson_drm *priv) return 0; }
-static int meson_g12a_afbcd_reset(struct meson_drm *priv) +static void meson_g12a_afbcd_exit(struct meson_drm *priv) {
- meson_rdma_reset(priv);
- meson_rdma_writel_sync(priv, VIU_SW_RESET_G12A_AFBC_ARB |
VIU_SW_RESET_G12A_OSD1_AFBCD,VIU_SW_RESET);- meson_rdma_writel_sync(priv, 0, VIU_SW_RESET);
- return 0;
- meson_g12a_afbcd_reset(priv);
- meson_rdma_free(priv);
}
static int meson_g12a_afbcd_enable(struct meson_drm *priv) @@ -380,6 +392,7 @@ static int meson_g12a_afbcd_setup(struct meson_drm *priv)
struct meson_afbcd_ops meson_afbcd_g12a_ops = { .init = meson_g12a_afbcd_init,
- .exit = meson_g12a_afbcd_exit, .reset = meson_g12a_afbcd_reset, .enable = meson_g12a_afbcd_enable, .disable = meson_g12a_afbcd_disable,
diff --git a/drivers/gpu/drm/meson/meson_osd_afbcd.h b/drivers/gpu/drm/meson/meson_osd_afbcd.h index 5e5523304f42..e77ddeb6416f 100644 --- a/drivers/gpu/drm/meson/meson_osd_afbcd.h +++ b/drivers/gpu/drm/meson/meson_osd_afbcd.h @@ -14,6 +14,7 @@
struct meson_afbcd_ops { int (*init)(struct meson_drm *priv);
- void (*exit)(struct meson_drm *priv); int (*reset)(struct meson_drm *priv); int (*enable)(struct meson_drm *priv); int (*disable)(struct meson_drm *priv);
Acked-by: Neil Armstrong narmstrong@baylibre.com
Thanks for fixing this
Neil
When afbcd.ops->init fails we need to free the struct drm_device. Also all errors which come after afbcd.ops->init was successful need to exit the AFBCD, just like meson_drv_unbind() does.
Fixes: d1b5e41e13a7e9 ("drm/meson: Add AFBCD module driver") Signed-off-by: Martin Blumenstingl martin.blumenstingl@googlemail.com --- drivers/gpu/drm/meson/meson_drv.c | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/meson/meson_drv.c b/drivers/gpu/drm/meson/meson_drv.c index b919271a6e50..26aeaf0ab86e 100644 --- a/drivers/gpu/drm/meson/meson_drv.c +++ b/drivers/gpu/drm/meson/meson_drv.c @@ -302,42 +302,42 @@ static int meson_drv_bind_master(struct device *dev, bool has_components) if (priv->afbcd.ops) { ret = priv->afbcd.ops->init(priv); if (ret) - return ret; + goto free_drm; }
/* Encoder Initialization */
ret = meson_encoder_cvbs_init(priv); if (ret) - goto free_drm; + goto exit_afbcd;
if (has_components) { ret = component_bind_all(drm->dev, drm); if (ret) { dev_err(drm->dev, "Couldn't bind all components\n"); - goto free_drm; + goto exit_afbcd; } }
ret = meson_encoder_hdmi_init(priv); if (ret) - goto free_drm; + goto exit_afbcd;
ret = meson_plane_create(priv); if (ret) - goto free_drm; + goto exit_afbcd;
ret = meson_overlay_create(priv); if (ret) - goto free_drm; + goto exit_afbcd;
ret = meson_crtc_create(priv); if (ret) - goto free_drm; + goto exit_afbcd;
ret = request_irq(priv->vsync_irq, meson_irq, 0, drm->driver->name, drm); if (ret) - goto free_drm; + goto exit_afbcd;
drm_mode_config_reset(drm);
@@ -355,6 +355,9 @@ static int meson_drv_bind_master(struct device *dev, bool has_components)
uninstall_irq: free_irq(priv->vsync_irq, drm); +exit_afbcd: + if (priv->afbcd.ops) + priv->afbcd.ops->exit(priv); free_drm: drm_dev_put(drm);
On 31/12/2021 00:55, Martin Blumenstingl wrote:
When afbcd.ops->init fails we need to free the struct drm_device. Also all errors which come after afbcd.ops->init was successful need to exit the AFBCD, just like meson_drv_unbind() does.
Fixes: d1b5e41e13a7e9 ("drm/meson: Add AFBCD module driver") Signed-off-by: Martin Blumenstingl martin.blumenstingl@googlemail.com
drivers/gpu/drm/meson/meson_drv.c | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/meson/meson_drv.c b/drivers/gpu/drm/meson/meson_drv.c index b919271a6e50..26aeaf0ab86e 100644 --- a/drivers/gpu/drm/meson/meson_drv.c +++ b/drivers/gpu/drm/meson/meson_drv.c @@ -302,42 +302,42 @@ static int meson_drv_bind_master(struct device *dev, bool has_components) if (priv->afbcd.ops) { ret = priv->afbcd.ops->init(priv); if (ret)
return ret;
goto free_drm;}
/* Encoder Initialization */
ret = meson_encoder_cvbs_init(priv); if (ret)
goto free_drm;
goto exit_afbcd;if (has_components) { ret = component_bind_all(drm->dev, drm); if (ret) { dev_err(drm->dev, "Couldn't bind all components\n");
goto free_drm;
goto exit_afbcd;} }
ret = meson_encoder_hdmi_init(priv); if (ret)
goto free_drm;
goto exit_afbcd;ret = meson_plane_create(priv); if (ret)
goto free_drm;
goto exit_afbcd;ret = meson_overlay_create(priv); if (ret)
goto free_drm;
goto exit_afbcd;ret = meson_crtc_create(priv); if (ret)
goto free_drm;
goto exit_afbcd;ret = request_irq(priv->vsync_irq, meson_irq, 0, drm->driver->name, drm); if (ret)
goto free_drm;
goto exit_afbcd;drm_mode_config_reset(drm);
@@ -355,6 +355,9 @@ static int meson_drv_bind_master(struct device *dev, bool has_components)
uninstall_irq: free_irq(priv->vsync_irq, drm); +exit_afbcd:
- if (priv->afbcd.ops)
priv->afbcd.ops->exit(priv);free_drm: drm_dev_put(drm);
Acked-by: Neil Armstrong narmstrong@baylibre.com
Thanks,
But this depends on patch 1, so I'll add the same Fixes tag on patch 1 and add to drm-misc-next so it won't pollute -fixes but will still be eventually be backported when landing on linus master.
Neil
dri-devel@lists.freedesktop.org
-
Martin Blumenstingl -
Neil Armstrong