| Message ID | df4099d79c985c73bdc890eb0e026494b7fa5c96.1680273039.git.geert+renesas@glider.be (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm: shmobile: Fixes and enhancements | expand |
Hi Geert, Thank you for the patch. On Fri, Mar 31, 2023 at 04:48:09PM +0200, Geert Uytterhoeven wrote: > The SH-Mobile DRM driver uses the legacy drm_crtc_init(), which > advertizes only the formats in safe_modeset_formats[] (XR24 and AR24) as > being supported. > > Switch to drm_crtc_init_with_planes(), and advertize all supported > (A)RGB modes, so we can use RGB565 as the default mode for the console. > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> > --- > drivers/gpu/drm/shmobile/shmob_drm_crtc.c | 30 +++++++++++++++++++++-- > 1 file changed, 28 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/shmobile/shmob_drm_crtc.c b/drivers/gpu/drm/shmobile/shmob_drm_crtc.c > index 08dc1428aa16caf0..11dd2bc803e7cb62 100644 > --- a/drivers/gpu/drm/shmobile/shmob_drm_crtc.c > +++ b/drivers/gpu/drm/shmobile/shmob_drm_crtc.c > @@ -18,6 +18,7 @@ > #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> > @@ -478,16 +479,41 @@ 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, > +}; > + > +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; > int ret; > > sdev->crtc.dpms = DRM_MODE_DPMS_OFF; > > - ret = drm_crtc_init(sdev->ddev, crtc, &crtc_funcs); > - if (ret < 0) > + 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); > + if (IS_ERR(primary)) > + return PTR_ERR(primary); This seems like a bit of a hack to me. Why don't you use the planes created by shmob_drm_plane_create() instead of allocating a new one ? > + > + ret = drm_crtc_init_with_planes(sdev->ddev, crtc, primary, NULL, > + &crtc_funcs, NULL); > + if (ret < 0) { > + drm_plane_cleanup(primary); > + kfree(primary); > return ret; > + } > > drm_crtc_helper_add(crtc, &crtc_helper_funcs); >
Hi Laurent, Thanks for your comments! On Wed, Apr 5, 2023 at 5:59 AM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > On Fri, Mar 31, 2023 at 04:48:09PM +0200, Geert Uytterhoeven wrote: > > The SH-Mobile DRM driver uses the legacy drm_crtc_init(), which > > advertizes only the formats in safe_modeset_formats[] (XR24 and AR24) as > > being supported. > > > > Switch to drm_crtc_init_with_planes(), and advertize all supported > > (A)RGB modes, so we can use RGB565 as the default mode for the console. > > > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> > > --- a/drivers/gpu/drm/shmobile/shmob_drm_crtc.c > > +++ b/drivers/gpu/drm/shmobile/shmob_drm_crtc.c > > @@ -18,6 +18,7 @@ > > #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> > > @@ -478,16 +479,41 @@ 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, > > +}; > > + > > +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; > > int ret; > > > > sdev->crtc.dpms = DRM_MODE_DPMS_OFF; > > > > - ret = drm_crtc_init(sdev->ddev, crtc, &crtc_funcs); > > - if (ret < 0) > > + 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); > > + if (IS_ERR(primary)) > > + return PTR_ERR(primary); > > This seems like a bit of a hack to me. Why don't you use the planes I'm following what Thomas did in the nouveau driver.... > created by shmob_drm_plane_create() instead of allocating a new one ? Is that possible? shmob_drm_plane_create() creates overlay planes, while this is for the primary plane. > > > + > > + ret = drm_crtc_init_with_planes(sdev->ddev, crtc, primary, NULL, > > + &crtc_funcs, NULL); > > + if (ret < 0) { > > + drm_plane_cleanup(primary); > > + kfree(primary); > > return ret; > > + } > > > > drm_crtc_helper_add(crtc, &crtc_helper_funcs); Gr{oetje,eeting}s, Geert
Hi Geert, On Mon, Apr 10, 2023 at 11:35:56AM +0200, Geert Uytterhoeven wrote: > On Wed, Apr 5, 2023 at 5:59 AM Laurent Pinchart wrote: > > On Fri, Mar 31, 2023 at 04:48:09PM +0200, Geert Uytterhoeven wrote: > > > The SH-Mobile DRM driver uses the legacy drm_crtc_init(), which > > > advertizes only the formats in safe_modeset_formats[] (XR24 and AR24) as > > > being supported. > > > > > > Switch to drm_crtc_init_with_planes(), and advertize all supported > > > (A)RGB modes, so we can use RGB565 as the default mode for the console. > > > > > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> > > > > --- a/drivers/gpu/drm/shmobile/shmob_drm_crtc.c > > > +++ b/drivers/gpu/drm/shmobile/shmob_drm_crtc.c > > > @@ -18,6 +18,7 @@ > > > #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> > > > @@ -478,16 +479,41 @@ 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, > > > +}; > > > + > > > +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; > > > int ret; > > > > > > sdev->crtc.dpms = DRM_MODE_DPMS_OFF; > > > > > > - ret = drm_crtc_init(sdev->ddev, crtc, &crtc_funcs); > > > - if (ret < 0) > > > + 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); > > > + if (IS_ERR(primary)) > > > + return PTR_ERR(primary); > > > > This seems like a bit of a hack to me. Why don't you use the planes > > I'm following what Thomas did in the nouveau driver.... > > > created by shmob_drm_plane_create() instead of allocating a new one ? > > Is that possible? shmob_drm_plane_create() creates overlay planes, > while this is for the primary plane. You're right, for some reason I overlooked that. Sorry for the noise. It would be good to move handling of the primary plane to shmob_drm_plane.c, but that's for later, when moving the driver to the atomic API. For now, Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> > > > + > > > + ret = drm_crtc_init_with_planes(sdev->ddev, crtc, primary, NULL, > > > + &crtc_funcs, NULL); > > > + if (ret < 0) { > > > + drm_plane_cleanup(primary); > > > + kfree(primary); > > > return ret; > > > + } > > > > > > drm_crtc_helper_add(crtc, &crtc_helper_funcs);
diff --git a/drivers/gpu/drm/shmobile/shmob_drm_crtc.c b/drivers/gpu/drm/shmobile/shmob_drm_crtc.c index 08dc1428aa16caf0..11dd2bc803e7cb62 100644 --- a/drivers/gpu/drm/shmobile/shmob_drm_crtc.c +++ b/drivers/gpu/drm/shmobile/shmob_drm_crtc.c @@ -18,6 +18,7 @@ #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> @@ -478,16 +479,41 @@ 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, +}; + +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; int ret; sdev->crtc.dpms = DRM_MODE_DPMS_OFF; - ret = drm_crtc_init(sdev->ddev, crtc, &crtc_funcs); - if (ret < 0) + 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); + if (IS_ERR(primary)) + return PTR_ERR(primary); + + ret = drm_crtc_init_with_planes(sdev->ddev, crtc, primary, NULL, + &crtc_funcs, NULL); + if (ret < 0) { + drm_plane_cleanup(primary); + kfree(primary); return ret; + } drm_crtc_helper_add(crtc, &crtc_helper_funcs);
The SH-Mobile DRM driver uses the legacy drm_crtc_init(), which advertizes only the formats in safe_modeset_formats[] (XR24 and AR24) as being supported. Switch to drm_crtc_init_with_planes(), and advertize all supported (A)RGB modes, so we can use RGB565 as the default mode for the console. Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> --- drivers/gpu/drm/shmobile/shmob_drm_crtc.c | 30 +++++++++++++++++++++-- 1 file changed, 28 insertions(+), 2 deletions(-)