[i-g-t,7/8] igt: fb: Fallback on KMS dumb buffer allocation for YUV buffers

Submitted by Maxime Ripard on Dec. 4, 2018, 10:08 a.m.

Details

Message ID 20181204100826.15522-7-maxime.ripard@bootlin.com
State New
Headers show

Patch hide | download patch | download mbox

diff --git a/lib/igt_fb.c b/lib/igt_fb.c
index d6242a6652f1..f2e6c89f3884 100644
--- a/lib/igt_fb.c
+++ b/lib/igt_fb.c
@@ -501,6 +501,8 @@  static int i915_create_gem_for_fb(struct igt_fb *fb)
 
 static int create_yuv_bo_for_fb(struct igt_fb *fb)
 {
+	unsigned int virtual_height;
+	unsigned int bpp;
 	uint64_t size = calc_fb_size(fb);
 	int fd = fb->fd;
 
@@ -511,8 +513,37 @@  static int create_yuv_bo_for_fb(struct igt_fb *fb)
 	if (is_i915_device(fd))
 		return i915_create_gem_for_fb(fb);
 
-	/* We cannot allocate any other buffer type */
-	igt_assert(true);
+	switch (fb->drm_format) {
+	case DRM_FORMAT_NV12:
+		bpp = 8;
+		break;
+
+	case DRM_FORMAT_UYVY:
+	case DRM_FORMAT_VYUY:
+	case DRM_FORMAT_YUYV:
+	case DRM_FORMAT_YVYU:
+		bpp = 16;
+		break;
+
+	default:
+		igt_assert_f(false, "Unsupported YUV format\n");
+	}
+
+	switch (fb->drm_format) {
+	case DRM_FORMAT_NV12:
+		virtual_height = fb->height * 3 / 2;
+		break;
+
+	default:
+		virtual_height = fb->height;
+		break;
+	}
+
+	fb->is_dumb = true;
+	fb->gem_handle = kmstest_dumb_create(fd, fb->width, virtual_height,
+					     bpp, NULL, &fb->size);
+
+	return fb->gem_handle;
 }
 
 /* helpers to create nice-looking framebuffers */

Comments

Ville Syrjälä Dec. 4, 2018, 7:32 p.m.
On Tue, Dec 04, 2018 at 11:08:22AM +0100, Maxime Ripard wrote:
> The current YUV buffer allocation only works on the i915 driver, since
> it uses some private ioctl. However, we can to use that code on other
> drivers that implement only KMS, so if the driver is something else
> than the i915 driver, let's allocate a dumb buffer.
> 
> Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
> ---
>  lib/igt_fb.c | 35 +++++++++++++++++++++++++++++++++--
>  1 file changed, 33 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/igt_fb.c b/lib/igt_fb.c
> index d6242a6652f1..f2e6c89f3884 100644
> --- a/lib/igt_fb.c
> +++ b/lib/igt_fb.c
> @@ -501,6 +501,8 @@ static int i915_create_gem_for_fb(struct igt_fb *fb)
>  
>  static int create_yuv_bo_for_fb(struct igt_fb *fb)
>  {
> +	unsigned int virtual_height;
> +	unsigned int bpp;
>  	uint64_t size = calc_fb_size(fb);
>  	int fd = fb->fd;
>  
> @@ -511,8 +513,37 @@ static int create_yuv_bo_for_fb(struct igt_fb *fb)
>  	if (is_i915_device(fd))
>  		return i915_create_gem_for_fb(fb);
>  
> -	/* We cannot allocate any other buffer type */
> -	igt_assert(true);
> +	switch (fb->drm_format) {
> +	case DRM_FORMAT_NV12:
> +		bpp = 8;
> +		break;
> +
> +	case DRM_FORMAT_UYVY:
> +	case DRM_FORMAT_VYUY:
> +	case DRM_FORMAT_YUYV:
> +	case DRM_FORMAT_YVYU:
> +		bpp = 16;
> +		break;
> +
> +	default:
> +		igt_assert_f(false, "Unsupported YUV format\n");
> +	}
> +
> +	switch (fb->drm_format) {
> +	case DRM_FORMAT_NV12:
> +		virtual_height = fb->height * 3 / 2;
> +		break;

A bit of a hack. The main problem with this is that the kernel won't
know anything about any stride alignment limitations etc. If we do use
this hack I guess it would be more technically correct to write it as
DIV_ROUND_UP(fb->size, fb->stride[0]), or something along those lines.

Which also brings me to the other issue we have. calc_plane_stride()
and calc_plane_size() are somewhat i915 specific. We should probably
do something about that.

> +
> +	default:
> +		virtual_height = fb->height;
> +		break;
> +	}
> +
> +	fb->is_dumb = true;
> +	fb->gem_handle = kmstest_dumb_create(fd, fb->width, virtual_height,
> +					     bpp, NULL, &fb->size);
> +
> +	return fb->gem_handle;
>  }
>  
>  /* helpers to create nice-looking framebuffers */
> -- 
> 2.19.1
Maxime Ripard Dec. 7, 2018, 3:29 p.m.
On Tue, Dec 04, 2018 at 09:32:05PM +0200, Ville Syrjälä wrote:
> On Tue, Dec 04, 2018 at 11:08:22AM +0100, Maxime Ripard wrote:
> > The current YUV buffer allocation only works on the i915 driver, since
> > it uses some private ioctl. However, we can to use that code on other
> > drivers that implement only KMS, so if the driver is something else
> > than the i915 driver, let's allocate a dumb buffer.
> > 
> > Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
> > ---
> >  lib/igt_fb.c | 35 +++++++++++++++++++++++++++++++++--
> >  1 file changed, 33 insertions(+), 2 deletions(-)
> > 
> > diff --git a/lib/igt_fb.c b/lib/igt_fb.c
> > index d6242a6652f1..f2e6c89f3884 100644
> > --- a/lib/igt_fb.c
> > +++ b/lib/igt_fb.c
> > @@ -501,6 +501,8 @@ static int i915_create_gem_for_fb(struct igt_fb *fb)
> >  
> >  static int create_yuv_bo_for_fb(struct igt_fb *fb)
> >  {
> > +	unsigned int virtual_height;
> > +	unsigned int bpp;
> >  	uint64_t size = calc_fb_size(fb);
> >  	int fd = fb->fd;
> >  
> > @@ -511,8 +513,37 @@ static int create_yuv_bo_for_fb(struct igt_fb *fb)
> >  	if (is_i915_device(fd))
> >  		return i915_create_gem_for_fb(fb);
> >  
> > -	/* We cannot allocate any other buffer type */
> > -	igt_assert(true);
> > +	switch (fb->drm_format) {
> > +	case DRM_FORMAT_NV12:
> > +		bpp = 8;
> > +		break;
> > +
> > +	case DRM_FORMAT_UYVY:
> > +	case DRM_FORMAT_VYUY:
> > +	case DRM_FORMAT_YUYV:
> > +	case DRM_FORMAT_YVYU:
> > +		bpp = 16;
> > +		break;
> > +
> > +	default:
> > +		igt_assert_f(false, "Unsupported YUV format\n");
> > +	}
> > +
> > +	switch (fb->drm_format) {
> > +	case DRM_FORMAT_NV12:
> > +		virtual_height = fb->height * 3 / 2;
> > +		break;
> 
> A bit of a hack. The main problem with this is that the kernel won't
> know anything about any stride alignment limitations etc. If we do use
> this hack I guess it would be more technically correct to write it as
> DIV_ROUND_UP(fb->size, fb->stride[0]), or something along those lines.

It's the same algorithm that modetest is using, which is why I went
for that. It must have been tested on a wide variety of platforms.

> Which also brings me to the other issue we have. calc_plane_stride()
> and calc_plane_size() are somewhat i915 specific. We should probably
> do something about that.

Ah, right, I missed that.

When will we start to put some intel specific code in a generic code
path?

Maxime
Ville Syrjälä Dec. 7, 2018, 5:39 p.m.
On Fri, Dec 07, 2018 at 04:29:54PM +0100, Maxime Ripard wrote:
> On Tue, Dec 04, 2018 at 09:32:05PM +0200, Ville Syrjälä wrote:
> > On Tue, Dec 04, 2018 at 11:08:22AM +0100, Maxime Ripard wrote:
> > > The current YUV buffer allocation only works on the i915 driver, since
> > > it uses some private ioctl. However, we can to use that code on other
> > > drivers that implement only KMS, so if the driver is something else
> > > than the i915 driver, let's allocate a dumb buffer.
> > > 
> > > Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
> > > ---
> > >  lib/igt_fb.c | 35 +++++++++++++++++++++++++++++++++--
> > >  1 file changed, 33 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/lib/igt_fb.c b/lib/igt_fb.c
> > > index d6242a6652f1..f2e6c89f3884 100644
> > > --- a/lib/igt_fb.c
> > > +++ b/lib/igt_fb.c
> > > @@ -501,6 +501,8 @@ static int i915_create_gem_for_fb(struct igt_fb *fb)
> > >  
> > >  static int create_yuv_bo_for_fb(struct igt_fb *fb)
> > >  {
> > > +	unsigned int virtual_height;
> > > +	unsigned int bpp;
> > >  	uint64_t size = calc_fb_size(fb);
> > >  	int fd = fb->fd;
> > >  
> > > @@ -511,8 +513,37 @@ static int create_yuv_bo_for_fb(struct igt_fb *fb)
> > >  	if (is_i915_device(fd))
> > >  		return i915_create_gem_for_fb(fb);
> > >  
> > > -	/* We cannot allocate any other buffer type */
> > > -	igt_assert(true);
> > > +	switch (fb->drm_format) {
> > > +	case DRM_FORMAT_NV12:
> > > +		bpp = 8;
> > > +		break;
> > > +
> > > +	case DRM_FORMAT_UYVY:
> > > +	case DRM_FORMAT_VYUY:
> > > +	case DRM_FORMAT_YUYV:
> > > +	case DRM_FORMAT_YVYU:
> > > +		bpp = 16;
> > > +		break;
> > > +
> > > +	default:
> > > +		igt_assert_f(false, "Unsupported YUV format\n");
> > > +	}
> > > +
> > > +	switch (fb->drm_format) {
> > > +	case DRM_FORMAT_NV12:
> > > +		virtual_height = fb->height * 3 / 2;
> > > +		break;
> > 
> > A bit of a hack. The main problem with this is that the kernel won't
> > know anything about any stride alignment limitations etc. If we do use
> > this hack I guess it would be more technically correct to write it as
> > DIV_ROUND_UP(fb->size, fb->stride[0]), or something along those lines.
> 
> It's the same algorithm that modetest is using, which is why I went
> for that. It must have been tested on a wide variety of platforms.

People actually use modetest?

Anyways, I guess it's sort of correct in the sense that we override
fb->strides[0] and fb->size based on the data returned from the
crerate_dumb ioctl. Though DIV_ROUND_UP(height * 3, 2) would
seem like a sane idea anyway.

But what's missing is the handling for the nv12 chroma plane. We'd
probably want to rewrite offsets[1] and strides[1] as well. Although
we'll still be totally guessing since the kernel wasn't told that
we want NV12 and so there's is no guarantee that it'll accept the
resulting fb.

> 
> > Which also brings me to the other issue we have. calc_plane_stride()
> > and calc_plane_size() are somewhat i915 specific. We should probably
> > do something about that.
> 
> Ah, right, I missed that.
> 
> When will we start to put some intel specific code in a generic code
> path?

Do you mean stop? Hopefully we've already stopped.