Hi Geert, Thank you for the patch.
On Thu, Jun 22, 2023 at 11:21:36AM +0200, Geert Uytterhoeven wrote: > Unify primary and overlay plane allocation: > - Enhance shmob_drm_plane_create() so it can be used to create the > primary plane, too, > - Move overlay plane creation next to primary plane creation. > > As overlay plane index zero now means the primary plane, this requires > shifting all overlay plane indices by one. Do you use index zero to identify the primary plane just for shmob_drm_plane_create(), or somewhere else too ? If it's just to create the plane, you could instead pass the plane type to the function. > Signed-off-by: Geert Uytterhoeven <[email protected]> > --- > Perhaps it would be better to not use dynamic allocation, but store > "struct drm_plane primary" and "struct shmob_drm_plane planes[5]" in > struct drm_shmob_device instead, like is done for the crtc and encoder? > --- > .../gpu/drm/renesas/shmobile/shmob_drm_crtc.c | 39 ++------ > .../gpu/drm/renesas/shmobile/shmob_drm_drv.c | 9 -- > .../drm/renesas/shmobile/shmob_drm_plane.c | 96 +++++++++++-------- > .../drm/renesas/shmobile/shmob_drm_plane.h | 3 +- > 4 files changed, 70 insertions(+), 77 deletions(-) > > diff --git a/drivers/gpu/drm/renesas/shmobile/shmob_drm_crtc.c > b/drivers/gpu/drm/renesas/shmobile/shmob_drm_crtc.c > index 28a70536693f7788..3f6af12f45988124 100644 > --- a/drivers/gpu/drm/renesas/shmobile/shmob_drm_crtc.c > +++ b/drivers/gpu/drm/renesas/shmobile/shmob_drm_crtc.c > @@ -19,7 +19,6 @@ > #include <drm/drm_gem_dma_helper.h> > #include <drm/drm_modeset_helper.h> > #include <drm/drm_modeset_helper_vtables.h> > -#include <drm/drm_plane_helper.h> > #include <drm/drm_probe_helper.h> > #include <drm/drm_simple_kms_helper.h> > #include <drm/drm_vblank.h> > @@ -501,47 +500,29 @@ static const struct drm_crtc_funcs crtc_funcs = { > .disable_vblank = shmob_drm_disable_vblank, > }; > > -static const uint32_t modeset_formats[] = { > - DRM_FORMAT_RGB565, > - DRM_FORMAT_RGB888, > - DRM_FORMAT_ARGB8888, > - DRM_FORMAT_XRGB8888, > - DRM_FORMAT_NV12, > - DRM_FORMAT_NV21, > - DRM_FORMAT_NV16, > - DRM_FORMAT_NV61, > - DRM_FORMAT_NV24, > - DRM_FORMAT_NV42, > -}; > - > -static const struct drm_plane_funcs primary_plane_funcs = { > - DRM_PLANE_NON_ATOMIC_FUNCS, > -}; > - > int shmob_drm_crtc_create(struct shmob_drm_device *sdev) > { > struct drm_crtc *crtc = &sdev->crtc.crtc; > - struct drm_plane *primary; > + struct drm_plane *primary, *plane; > + unsigned int i; > int ret; > > sdev->crtc.dpms = DRM_MODE_DPMS_OFF; > > - primary = __drm_universal_plane_alloc(&sdev->ddev, sizeof(*primary), 0, > - 0, &primary_plane_funcs, > - modeset_formats, > - ARRAY_SIZE(modeset_formats), > - NULL, DRM_PLANE_TYPE_PRIMARY, > - NULL); > + primary = shmob_drm_plane_create(sdev, 0); > if (IS_ERR(primary)) > return PTR_ERR(primary); > > + for (i = 1; i < 5; ++i) { > + plane = shmob_drm_plane_create(sdev, i); > + if (IS_ERR(plane)) > + return PTR_ERR(plane); > + } > + > ret = drm_crtc_init_with_planes(&sdev->ddev, crtc, primary, NULL, > &crtc_funcs, NULL); > - if (ret < 0) { > - drm_plane_cleanup(primary); > - kfree(primary); > + if (ret < 0) > return ret; > - } > > drm_crtc_helper_add(crtc, &crtc_helper_funcs); > > diff --git a/drivers/gpu/drm/renesas/shmobile/shmob_drm_drv.c > b/drivers/gpu/drm/renesas/shmobile/shmob_drm_drv.c > index c15ebbe74cac501f..c98e2bdd888c3274 100644 > --- a/drivers/gpu/drm/renesas/shmobile/shmob_drm_drv.c > +++ b/drivers/gpu/drm/renesas/shmobile/shmob_drm_drv.c > @@ -154,7 +154,6 @@ static int shmob_drm_probe(struct platform_device *pdev) > struct shmob_drm_platform_data *pdata = pdev->dev.platform_data; > struct shmob_drm_device *sdev; > struct drm_device *ddev; > - unsigned int i; > int ret; > > if (pdata == NULL) { > @@ -195,14 +194,6 @@ static int shmob_drm_probe(struct platform_device *pdev) > return dev_err_probe(&pdev->dev, ret, > "failed to initialize mode setting\n"); > > - for (i = 0; i < 4; ++i) { > - ret = shmob_drm_plane_create(sdev, i); > - if (ret < 0) { > - dev_err(&pdev->dev, "failed to create plane %u\n", i); > - goto err_modeset_cleanup; > - } > - } > - > ret = drm_vblank_init(ddev, 1); > if (ret < 0) { > dev_err(&pdev->dev, "failed to initialize vblank\n"); > diff --git a/drivers/gpu/drm/renesas/shmobile/shmob_drm_plane.c > b/drivers/gpu/drm/renesas/shmobile/shmob_drm_plane.c > index c58b9dca34736342..63886015baaebfc0 100644 > --- a/drivers/gpu/drm/renesas/shmobile/shmob_drm_plane.c > +++ b/drivers/gpu/drm/renesas/shmobile/shmob_drm_plane.c > @@ -12,6 +12,7 @@ > #include <drm/drm_fourcc.h> > #include <drm/drm_framebuffer.h> > #include <drm/drm_gem_dma_helper.h> > +#include <drm/drm_plane_helper.h> > > #include "shmob_drm_drv.h" > #include "shmob_drm_kms.h" > @@ -64,57 +65,58 @@ static void __shmob_drm_plane_setup(struct > shmob_drm_plane *splane, > struct drm_framebuffer *fb) > { > struct shmob_drm_device *sdev = to_shmob_device(splane->plane.dev); > + unsigned int ovl_idx = splane->index - 1; > u32 format; > > /* TODO: Support ROP3 mode */ > format = LDBBSIFR_EN | (splane->alpha << LDBBSIFR_LAY_SHIFT) | > splane->format->ldbbsifr; > > -#define plane_reg_dump(sdev, splane, reg) \ > +#define plane_reg_dump(sdev, ovl_idx, reg) \ > dev_dbg(sdev->ddev.dev, "%s(%u): %s 0x%08x 0x%08x\n", __func__, \ > - splane->index, #reg, \ > - lcdc_read(sdev, reg(splane->index)), \ > - lcdc_read(sdev, reg(splane->index) + LCDC_SIDE_B_OFFSET)) > - > - plane_reg_dump(sdev, splane, LDBnBSIFR); > - plane_reg_dump(sdev, splane, LDBnBSSZR); > - plane_reg_dump(sdev, splane, LDBnBLOCR); > - plane_reg_dump(sdev, splane, LDBnBSMWR); > - plane_reg_dump(sdev, splane, LDBnBSAYR); > - plane_reg_dump(sdev, splane, LDBnBSACR); > - > - lcdc_write(sdev, LDBCR, LDBCR_UPC(splane->index)); > - dev_dbg(sdev->ddev.dev, "%s(%u): %s 0x%08x\n", __func__, splane->index, > + ovl_idx, #reg, \ > + lcdc_read(sdev, reg(ovl_idx)), \ > + lcdc_read(sdev, reg(ovl_idx) + LCDC_SIDE_B_OFFSET)) > + > + plane_reg_dump(sdev, ovl_idx, LDBnBSIFR); > + plane_reg_dump(sdev, ovl_idx, LDBnBSSZR); > + plane_reg_dump(sdev, ovl_idx, LDBnBLOCR); > + plane_reg_dump(sdev, ovl_idx, LDBnBSMWR); > + plane_reg_dump(sdev, ovl_idx, LDBnBSAYR); > + plane_reg_dump(sdev, ovl_idx, LDBnBSACR); > + > + lcdc_write(sdev, LDBCR, LDBCR_UPC(ovl_idx)); > + dev_dbg(sdev->ddev.dev, "%s(%u): %s 0x%08x\n", __func__, ovl_idx, > "LDBCR", lcdc_read(sdev, LDBCR)); > > - lcdc_write(sdev, LDBnBSIFR(splane->index), format); > + lcdc_write(sdev, LDBnBSIFR(ovl_idx), format); > > - lcdc_write(sdev, LDBnBSSZR(splane->index), > + lcdc_write(sdev, LDBnBSSZR(ovl_idx), > (splane->crtc_h << LDBBSSZR_BVSS_SHIFT) | > (splane->crtc_w << LDBBSSZR_BHSS_SHIFT)); > - lcdc_write(sdev, LDBnBLOCR(splane->index), > + lcdc_write(sdev, LDBnBLOCR(ovl_idx), > (splane->crtc_y << LDBBLOCR_CVLC_SHIFT) | > (splane->crtc_x << LDBBLOCR_CHLC_SHIFT)); > - lcdc_write(sdev, LDBnBSMWR(splane->index), > + lcdc_write(sdev, LDBnBSMWR(ovl_idx), > fb->pitches[0] << LDBBSMWR_BSMW_SHIFT); > > shmob_drm_plane_compute_base(splane, fb, splane->src_x, splane->src_y); > > - lcdc_write(sdev, LDBnBSAYR(splane->index), splane->dma[0]); > + lcdc_write(sdev, LDBnBSAYR(ovl_idx), splane->dma[0]); > if (shmob_drm_format_is_yuv(splane->format)) > - lcdc_write(sdev, LDBnBSACR(splane->index), splane->dma[1]); > + lcdc_write(sdev, LDBnBSACR(ovl_idx), splane->dma[1]); > > lcdc_write(sdev, LDBCR, > - LDBCR_UPF(splane->index) | LDBCR_UPD(splane->index)); > - dev_dbg(sdev->ddev.dev, "%s(%u): %s 0x%08x\n", __func__, splane->index, > + LDBCR_UPF(ovl_idx) | LDBCR_UPD(ovl_idx)); > + dev_dbg(sdev->ddev.dev, "%s(%u): %s 0x%08x\n", __func__, ovl_idx, > "LDBCR", lcdc_read(sdev, LDBCR)); > > - plane_reg_dump(sdev, splane, LDBnBSIFR); > - plane_reg_dump(sdev, splane, LDBnBSSZR); > - plane_reg_dump(sdev, splane, LDBnBLOCR); > - plane_reg_dump(sdev, splane, LDBnBSMWR); > - plane_reg_dump(sdev, splane, LDBnBSAYR); > - plane_reg_dump(sdev, splane, LDBnBSACR); > + plane_reg_dump(sdev, ovl_idx, LDBnBSIFR); > + plane_reg_dump(sdev, ovl_idx, LDBnBSSZR); > + plane_reg_dump(sdev, ovl_idx, LDBnBLOCR); > + plane_reg_dump(sdev, ovl_idx, LDBnBSMWR); > + plane_reg_dump(sdev, ovl_idx, LDBnBSAYR); > + plane_reg_dump(sdev, ovl_idx, LDBnBSACR); > } > > void shmob_drm_plane_setup(struct drm_plane *plane) > @@ -169,16 +171,22 @@ static int shmob_drm_plane_disable(struct drm_plane > *plane, > { > struct shmob_drm_device *sdev = to_shmob_device(plane->dev); > struct shmob_drm_plane *splane = to_shmob_plane(plane); > + unsigned int ovl_idx = splane->index - 1; > > splane->format = NULL; > > - lcdc_write(sdev, LDBCR, LDBCR_UPC(splane->index)); > - lcdc_write(sdev, LDBnBSIFR(splane->index), 0); > + lcdc_write(sdev, LDBCR, LDBCR_UPC(ovl_idx)); > + lcdc_write(sdev, LDBnBSIFR(ovl_idx), 0); > lcdc_write(sdev, LDBCR, > - LDBCR_UPF(splane->index) | LDBCR_UPD(splane->index)); > + LDBCR_UPF(ovl_idx) | LDBCR_UPD(ovl_idx)); > return 0; > } > > +static const struct drm_plane_funcs primary_plane_funcs = { While at it, you can name this shmob_primary_plane_funcs to avoid namespace clashes. > + .update_plane = drm_plane_helper_update_primary, > + .disable_plane = drm_plane_helper_disable_primary, > +}; > + > static const struct drm_plane_funcs shmob_drm_plane_funcs = { And shmob_overlay_plane_funcs for consistency. > .update_plane = shmob_drm_plane_update, > .disable_plane = shmob_drm_plane_disable, > @@ -197,19 +205,31 @@ static const uint32_t formats[] = { > DRM_FORMAT_NV42, > }; > > -int shmob_drm_plane_create(struct shmob_drm_device *sdev, unsigned int index) > +struct drm_plane *shmob_drm_plane_create(struct shmob_drm_device *sdev, > + unsigned int index) > { > + const struct drm_plane_funcs *funcs; > struct shmob_drm_plane *splane; > + enum drm_plane_type type; > + > + if (!index) { > + type = DRM_PLANE_TYPE_PRIMARY; > + funcs = &primary_plane_funcs; > + } else { > + type = DRM_PLANE_TYPE_OVERLAY; > + funcs = &shmob_drm_plane_funcs; > + } > > - splane = drmm_universal_plane_alloc(&sdev->ddev, struct shmob_drm_plane, > - plane, 1, &shmob_drm_plane_funcs, > - formats, ARRAY_SIZE(formats), NULL, > - DRM_PLANE_TYPE_OVERLAY, NULL); > + splane = drmm_universal_plane_alloc(&sdev->ddev, > + struct shmob_drm_plane, plane, 1, > + funcs, formats, > + ARRAY_SIZE(formats), NULL, type, > + NULL); > if (IS_ERR(splane)) > - return PTR_ERR(splane); > + return ERR_CAST(splane); > > splane->index = index; > splane->alpha = 255; > > - return 0; > + return &splane->plane; > } > diff --git a/drivers/gpu/drm/renesas/shmobile/shmob_drm_plane.h > b/drivers/gpu/drm/renesas/shmobile/shmob_drm_plane.h > index e72b21a4288fc23f..29cf6732d479a509 100644 > --- a/drivers/gpu/drm/renesas/shmobile/shmob_drm_plane.h > +++ b/drivers/gpu/drm/renesas/shmobile/shmob_drm_plane.h > @@ -13,7 +13,8 @@ > struct drm_plane; > struct shmob_drm_device; > > -int shmob_drm_plane_create(struct shmob_drm_device *sdev, unsigned int > index); > +struct drm_plane *shmob_drm_plane_create(struct shmob_drm_device *sdev, > + unsigned int index); > void shmob_drm_plane_setup(struct drm_plane *plane); > > #endif /* __SHMOB_DRM_PLANE_H__ */ -- Regards, Laurent Pinchart
