| Message ID | 95c2af42a89c65ca603126e56c0423407dfc873f.1687423204.git.geert+renesas@glider.be (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Kieran Bingham |
| Headers | show |
| Series | drm: renesas: shmobile: Atomic conversion + DT support | expand |
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 <geert+renesas@glider.be> > --- > 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__ */
Hi Laurent, On Fri, Jun 23, 2023 at 6:50 PM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > 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. Index zero is just used for the creation. I guess this sort of goes together with my question below... > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> > > --- > > 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? ... as embedding separate primary and planes[] would also get rid of the need to adjust the plane indices when converting from logical to physical overlay plane indices. Gr{oetje,eeting}s, Geert
On Fri, Jun 23, 2023 at 07:55:22PM +0200, Geert Uytterhoeven wrote: > On Fri, Jun 23, 2023 at 6:50 PM Laurent Pinchart wrote: > > 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. > > Index zero is just used for the creation. > I guess this sort of goes together with my question below... > > > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> > > > --- > > > 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? > > ... as embedding separate primary and planes[] would also get rid of > the need to adjust the plane indices when converting from logical to physical > overlay plane indices. Do they need to be embedded for that, or could you simple keep the index as it is ?
Hi Laurent, On Fri, Jun 23, 2023 at 8:50 PM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > On Fri, Jun 23, 2023 at 07:55:22PM +0200, Geert Uytterhoeven wrote: > > On Fri, Jun 23, 2023 at 6:50 PM Laurent Pinchart wrote: > > > 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. > > > > Index zero is just used for the creation. > > I guess this sort of goes together with my question below... > > > > > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> > > > > --- > > > > 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? > > > > ... as embedding separate primary and planes[] would also get rid of > > the need to adjust the plane indices when converting from logical to physical > > overlay plane indices. > > Do they need to be embedded for that, or could you simple keep the index > as it is ? If the plane type would be passed explicitly, they would not need to be embedded for that. Then the question becomes: does it make sense to unify primary and overlay plane handling? Gr{oetje,eeting}s, Geert
On Sun, Jun 25, 2023 at 10:58:17AM +0200, Geert Uytterhoeven wrote: > On Fri, Jun 23, 2023 at 8:50 PM Laurent Pinchart wrote: > > On Fri, Jun 23, 2023 at 07:55:22PM +0200, Geert Uytterhoeven wrote: > > > On Fri, Jun 23, 2023 at 6:50 PM Laurent Pinchart wrote: > > > > 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. > > > > > > Index zero is just used for the creation. > > > I guess this sort of goes together with my question below... > > > > > > > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> > > > > > --- > > > > > 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? > > > > > > ... as embedding separate primary and planes[] would also get rid of > > > the need to adjust the plane indices when converting from logical to physical > > > overlay plane indices. > > > > Do they need to be embedded for that, or could you simple keep the index > > as it is ? > > If the plane type would be passed explicitly, they would not need to be > embedded for that. > > Then the question becomes: does it make sense to unify primary and > overlay plane handling? Good point. I don't mind much either way, it depends on how much code duplication it would remove I suppose.
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 = { + .update_plane = drm_plane_helper_update_primary, + .disable_plane = drm_plane_helper_disable_primary, +}; + static const struct drm_plane_funcs shmob_drm_plane_funcs = { .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__ */
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. Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> --- 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(-)