[i-g-t,v2,04/13] igt: fb: Move i915 YUV buffer clearing code to a function

Submitted by Maxime Ripard on Jan. 8, 2019, 3:19 p.m.

Details

Message ID 20190108152001.5367-4-maxime.ripard@bootlin.com
State Accepted
Commit 49e44e8093d51c8ac020b441cdcca767a50db48b
Headers show
Series "igt: chamelium: Test YUV buffers using the Chamelium" ( rev: 4 3 ) in IGT

Not browsing as part of any series.

Commit Message

Maxime Ripard Jan. 8, 2019, 3:19 p.m.
The YUV buffer allocation path for the i915 driver also has some code to
clear a YUV buffer.

As that is going to be useful for all the other drivers, let's move it to
a separate function.

Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
---
 lib/igt_fb.c | 80 +++++++++++++++++++++++++++-------------------------
 1 file changed, 42 insertions(+), 38 deletions(-)

Patch hide | download patch | download mbox

diff --git a/lib/igt_fb.c b/lib/igt_fb.c
index 45691fd441d9..d69c3fb2d38d 100644
--- a/lib/igt_fb.c
+++ b/lib/igt_fb.c
@@ -484,6 +484,47 @@  uint64_t igt_fb_tiling_to_mod(uint64_t tiling)
 	}
 }
 
+static void clear_yuv_buffer(struct igt_fb *fb)
+{
+	bool full_range = fb->color_range == IGT_COLOR_YCBCR_FULL_RANGE;
+	void *ptr;
+
+	igt_assert(igt_format_is_yuv(fb->drm_format));
+
+	/* Ensure the framebuffer is preallocated */
+	ptr = igt_fb_map_buffer(fb->fd, fb);
+	igt_assert(*(uint32_t *)ptr == 0);
+
+	switch (fb->drm_format) {
+	case DRM_FORMAT_NV12:
+		memset(ptr + fb->offsets[0],
+		       full_range ? 0x00 : 0x10,
+		       fb->strides[0] * fb->plane_height[0]);
+		memset(ptr + fb->offsets[1],
+		       0x80,
+		       fb->strides[1] * fb->plane_height[1]);
+		break;
+	case DRM_FORMAT_XYUV8888:
+		wmemset(ptr + fb->offsets[0], full_range ? 0x00008080 : 0x00108080,
+			fb->strides[0] * fb->plane_height[0] / sizeof(wchar_t));
+		break;
+	case DRM_FORMAT_YUYV:
+	case DRM_FORMAT_YVYU:
+		wmemset(ptr + fb->offsets[0],
+			full_range ? 0x80008000 : 0x80108010,
+			fb->strides[0] * fb->plane_height[0] / sizeof(wchar_t));
+		break;
+	case DRM_FORMAT_UYVY:
+	case DRM_FORMAT_VYUY:
+		wmemset(ptr + fb->offsets[0],
+			full_range ? 0x00800080 : 0x10801080,
+			fb->strides[0] * fb->plane_height[0] / sizeof(wchar_t));
+		break;
+	}
+
+	igt_fb_unmap_buffer(fb, ptr);
+}
+
 /* helpers to create nice-looking framebuffers */
 static int create_bo_for_fb(struct igt_fb *fb)
 {
@@ -501,50 +542,13 @@  static int create_bo_for_fb(struct igt_fb *fb)
 		fb->is_dumb = false;
 
 		if (is_i915_device(fd)) {
-			void *ptr;
-			bool full_range = fb->color_range == IGT_COLOR_YCBCR_FULL_RANGE;
 
 			fb->gem_handle = gem_create(fd, fb->size);
 
 			gem_set_tiling(fd, fb->gem_handle,
 				       igt_fb_mod_to_tiling(fb->tiling),
 				       fb->strides[0]);
-
-			gem_set_domain(fd, fb->gem_handle,
-				       I915_GEM_DOMAIN_GTT, I915_GEM_DOMAIN_GTT);
-
-			/* Ensure the framebuffer is preallocated */
-			ptr = gem_mmap__gtt(fd, fb->gem_handle,
-					    fb->size, PROT_READ | PROT_WRITE);
-			igt_assert(*(uint32_t *)ptr == 0);
-
-			switch (fb->drm_format) {
-			case DRM_FORMAT_NV12:
-				memset(ptr + fb->offsets[0],
-				       full_range ? 0x00 : 0x10,
-				       fb->strides[0] * fb->plane_height[0]);
-				memset(ptr + fb->offsets[1],
-				       0x80,
-				       fb->strides[1] * fb->plane_height[1]);
-				break;
-			case DRM_FORMAT_XYUV8888:
-				wmemset(ptr + fb->offsets[0], full_range ? 0x00008080 : 0x00108080,
-					fb->strides[0] * fb->plane_height[0] / sizeof(wchar_t));
-				break;
-			case DRM_FORMAT_YUYV:
-			case DRM_FORMAT_YVYU:
-				wmemset(ptr + fb->offsets[0],
-					full_range ? 0x80008000 : 0x80108010,
-					fb->strides[0] * fb->plane_height[0] / sizeof(wchar_t));
-				break;
-			case DRM_FORMAT_UYVY:
-			case DRM_FORMAT_VYUY:
-				wmemset(ptr + fb->offsets[0],
-					full_range ? 0x00800080 : 0x10801080,
-					fb->strides[0] * fb->plane_height[0] / sizeof(wchar_t));
-				break;
-			}
-			gem_munmap(ptr, fb->size);
+			clear_yuv_buffer(fd);
 
 			return fb->gem_handle;
 		} else {

Comments

On Tue, 2019-01-08 at 16:19 +0100, Maxime Ripard wrote:
> The YUV buffer allocation path for the i915 driver also has some code to
> clear a YUV buffer.
> 
> As that is going to be useful for all the other drivers, let's move it to
> a separate function.
>
> Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>

I'm not really sure why clearing the buffers is a useful thing to do
and why we should only do it for YUV and not RGB formats.

Anyway, since this code was already there and the refactoring
definitely makes sense:

Reviewed-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>

Cheers,

Paul

> ---
>  lib/igt_fb.c | 80 +++++++++++++++++++++++++++-------------------------
>  1 file changed, 42 insertions(+), 38 deletions(-)
> 
> diff --git a/lib/igt_fb.c b/lib/igt_fb.c
> index 45691fd441d9..d69c3fb2d38d 100644
> --- a/lib/igt_fb.c
> +++ b/lib/igt_fb.c
> @@ -484,6 +484,47 @@ uint64_t igt_fb_tiling_to_mod(uint64_t tiling)
>  	}
>  }
>  
> +static void clear_yuv_buffer(struct igt_fb *fb)
> +{
> +	bool full_range = fb->color_range == IGT_COLOR_YCBCR_FULL_RANGE;
> +	void *ptr;
> +
> +	igt_assert(igt_format_is_yuv(fb->drm_format));
> +
> +	/* Ensure the framebuffer is preallocated */
> +	ptr = igt_fb_map_buffer(fb->fd, fb);
> +	igt_assert(*(uint32_t *)ptr == 0);
> +
> +	switch (fb->drm_format) {
> +	case DRM_FORMAT_NV12:
> +		memset(ptr + fb->offsets[0],
> +		       full_range ? 0x00 : 0x10,
> +		       fb->strides[0] * fb->plane_height[0]);
> +		memset(ptr + fb->offsets[1],
> +		       0x80,
> +		       fb->strides[1] * fb->plane_height[1]);
> +		break;
> +	case DRM_FORMAT_XYUV8888:
> +		wmemset(ptr + fb->offsets[0], full_range ? 0x00008080 : 0x00108080,
> +			fb->strides[0] * fb->plane_height[0] / sizeof(wchar_t));
> +		break;
> +	case DRM_FORMAT_YUYV:
> +	case DRM_FORMAT_YVYU:
> +		wmemset(ptr + fb->offsets[0],
> +			full_range ? 0x80008000 : 0x80108010,
> +			fb->strides[0] * fb->plane_height[0] / sizeof(wchar_t));
> +		break;
> +	case DRM_FORMAT_UYVY:
> +	case DRM_FORMAT_VYUY:
> +		wmemset(ptr + fb->offsets[0],
> +			full_range ? 0x00800080 : 0x10801080,
> +			fb->strides[0] * fb->plane_height[0] / sizeof(wchar_t));
> +		break;
> +	}
> +
> +	igt_fb_unmap_buffer(fb, ptr);
> +}
> +
>  /* helpers to create nice-looking framebuffers */
>  static int create_bo_for_fb(struct igt_fb *fb)
>  {
> @@ -501,50 +542,13 @@ static int create_bo_for_fb(struct igt_fb *fb)
>  		fb->is_dumb = false;
>  
>  		if (is_i915_device(fd)) {
> -			void *ptr;
> -			bool full_range = fb->color_range == IGT_COLOR_YCBCR_FULL_RANGE;
>  
>  			fb->gem_handle = gem_create(fd, fb->size);
>  
>  			gem_set_tiling(fd, fb->gem_handle,
>  				       igt_fb_mod_to_tiling(fb->tiling),
>  				       fb->strides[0]);
> -
> -			gem_set_domain(fd, fb->gem_handle,
> -				       I915_GEM_DOMAIN_GTT, I915_GEM_DOMAIN_GTT);
> -
> -			/* Ensure the framebuffer is preallocated */
> -			ptr = gem_mmap__gtt(fd, fb->gem_handle,
> -					    fb->size, PROT_READ | PROT_WRITE);
> -			igt_assert(*(uint32_t *)ptr == 0);
> -
> -			switch (fb->drm_format) {
> -			case DRM_FORMAT_NV12:
> -				memset(ptr + fb->offsets[0],
> -				       full_range ? 0x00 : 0x10,
> -				       fb->strides[0] * fb->plane_height[0]);
> -				memset(ptr + fb->offsets[1],
> -				       0x80,
> -				       fb->strides[1] * fb->plane_height[1]);
> -				break;
> -			case DRM_FORMAT_XYUV8888:
> -				wmemset(ptr + fb->offsets[0], full_range ? 0x00008080 : 0x00108080,
> -					fb->strides[0] * fb->plane_height[0] / sizeof(wchar_t));
> -				break;
> -			case DRM_FORMAT_YUYV:
> -			case DRM_FORMAT_YVYU:
> -				wmemset(ptr + fb->offsets[0],
> -					full_range ? 0x80008000 : 0x80108010,
> -					fb->strides[0] * fb->plane_height[0] / sizeof(wchar_t));
> -				break;
> -			case DRM_FORMAT_UYVY:
> -			case DRM_FORMAT_VYUY:
> -				wmemset(ptr + fb->offsets[0],
> -					full_range ? 0x00800080 : 0x10801080,
> -					fb->strides[0] * fb->plane_height[0] / sizeof(wchar_t));
> -				break;
> -			}
> -			gem_munmap(ptr, fb->size);
> +			clear_yuv_buffer(fd);
>  
>  			return fb->gem_handle;
>  		} else {
On Thu, Jan 10, 2019 at 11:10:09AM +0100, Paul Kocialkowski wrote:
> On Tue, 2019-01-08 at 16:19 +0100, Maxime Ripard wrote:
> > The YUV buffer allocation path for the i915 driver also has some code to
> > clear a YUV buffer.
> > 
> > As that is going to be useful for all the other drivers, let's move it to
> > a separate function.
> >
> > Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
> 
> I'm not really sure why clearing the buffers is a useful thing to do
> and why we should only do it for YUV and not RGB formats.

0 = black for RGB, 0 != black for YUV. The memset was just to make
things more consistent. But not sure if any test would actually
fail if we just drop the memset.

> 
> Anyway, since this code was already there and the refactoring
> definitely makes sense:
> 
> Reviewed-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> 
> Cheers,
> 
> Paul
> 
> > ---
> >  lib/igt_fb.c | 80 +++++++++++++++++++++++++++-------------------------
> >  1 file changed, 42 insertions(+), 38 deletions(-)
> > 
> > diff --git a/lib/igt_fb.c b/lib/igt_fb.c
> > index 45691fd441d9..d69c3fb2d38d 100644
> > --- a/lib/igt_fb.c
> > +++ b/lib/igt_fb.c
> > @@ -484,6 +484,47 @@ uint64_t igt_fb_tiling_to_mod(uint64_t tiling)
> >  	}
> >  }
> >  
> > +static void clear_yuv_buffer(struct igt_fb *fb)
> > +{
> > +	bool full_range = fb->color_range == IGT_COLOR_YCBCR_FULL_RANGE;
> > +	void *ptr;
> > +
> > +	igt_assert(igt_format_is_yuv(fb->drm_format));
> > +
> > +	/* Ensure the framebuffer is preallocated */
> > +	ptr = igt_fb_map_buffer(fb->fd, fb);
> > +	igt_assert(*(uint32_t *)ptr == 0);
> > +
> > +	switch (fb->drm_format) {
> > +	case DRM_FORMAT_NV12:
> > +		memset(ptr + fb->offsets[0],
> > +		       full_range ? 0x00 : 0x10,
> > +		       fb->strides[0] * fb->plane_height[0]);
> > +		memset(ptr + fb->offsets[1],
> > +		       0x80,
> > +		       fb->strides[1] * fb->plane_height[1]);
> > +		break;
> > +	case DRM_FORMAT_XYUV8888:
> > +		wmemset(ptr + fb->offsets[0], full_range ? 0x00008080 : 0x00108080,
> > +			fb->strides[0] * fb->plane_height[0] / sizeof(wchar_t));
> > +		break;
> > +	case DRM_FORMAT_YUYV:
> > +	case DRM_FORMAT_YVYU:
> > +		wmemset(ptr + fb->offsets[0],
> > +			full_range ? 0x80008000 : 0x80108010,
> > +			fb->strides[0] * fb->plane_height[0] / sizeof(wchar_t));
> > +		break;
> > +	case DRM_FORMAT_UYVY:
> > +	case DRM_FORMAT_VYUY:
> > +		wmemset(ptr + fb->offsets[0],
> > +			full_range ? 0x00800080 : 0x10801080,
> > +			fb->strides[0] * fb->plane_height[0] / sizeof(wchar_t));
> > +		break;
> > +	}
> > +
> > +	igt_fb_unmap_buffer(fb, ptr);
> > +}
> > +
> >  /* helpers to create nice-looking framebuffers */
> >  static int create_bo_for_fb(struct igt_fb *fb)
> >  {
> > @@ -501,50 +542,13 @@ static int create_bo_for_fb(struct igt_fb *fb)
> >  		fb->is_dumb = false;
> >  
> >  		if (is_i915_device(fd)) {
> > -			void *ptr;
> > -			bool full_range = fb->color_range == IGT_COLOR_YCBCR_FULL_RANGE;
> >  
> >  			fb->gem_handle = gem_create(fd, fb->size);
> >  
> >  			gem_set_tiling(fd, fb->gem_handle,
> >  				       igt_fb_mod_to_tiling(fb->tiling),
> >  				       fb->strides[0]);
> > -
> > -			gem_set_domain(fd, fb->gem_handle,
> > -				       I915_GEM_DOMAIN_GTT, I915_GEM_DOMAIN_GTT);
> > -
> > -			/* Ensure the framebuffer is preallocated */
> > -			ptr = gem_mmap__gtt(fd, fb->gem_handle,
> > -					    fb->size, PROT_READ | PROT_WRITE);
> > -			igt_assert(*(uint32_t *)ptr == 0);
> > -
> > -			switch (fb->drm_format) {
> > -			case DRM_FORMAT_NV12:
> > -				memset(ptr + fb->offsets[0],
> > -				       full_range ? 0x00 : 0x10,
> > -				       fb->strides[0] * fb->plane_height[0]);
> > -				memset(ptr + fb->offsets[1],
> > -				       0x80,
> > -				       fb->strides[1] * fb->plane_height[1]);
> > -				break;
> > -			case DRM_FORMAT_XYUV8888:
> > -				wmemset(ptr + fb->offsets[0], full_range ? 0x00008080 : 0x00108080,
> > -					fb->strides[0] * fb->plane_height[0] / sizeof(wchar_t));
> > -				break;
> > -			case DRM_FORMAT_YUYV:
> > -			case DRM_FORMAT_YVYU:
> > -				wmemset(ptr + fb->offsets[0],
> > -					full_range ? 0x80008000 : 0x80108010,
> > -					fb->strides[0] * fb->plane_height[0] / sizeof(wchar_t));
> > -				break;
> > -			case DRM_FORMAT_UYVY:
> > -			case DRM_FORMAT_VYUY:
> > -				wmemset(ptr + fb->offsets[0],
> > -					full_range ? 0x00800080 : 0x10801080,
> > -					fb->strides[0] * fb->plane_height[0] / sizeof(wchar_t));
> > -				break;
> > -			}
> > -			gem_munmap(ptr, fb->size);
> > +			clear_yuv_buffer(fd);
> >  
> >  			return fb->gem_handle;
> >  		} else {
> -- 
> Paul Kocialkowski, Bootlin
> Embedded Linux and kernel engineering
> https://bootlin.com
Hi,

On Thu, 2019-01-10 at 16:38 +0200, Ville Syrjälä wrote:
> On Thu, Jan 10, 2019 at 11:10:09AM +0100, Paul Kocialkowski wrote:
> > On Tue, 2019-01-08 at 16:19 +0100, Maxime Ripard wrote:
> > > The YUV buffer allocation path for the i915 driver also has some code to
> > > clear a YUV buffer.
> > > 
> > > As that is going to be useful for all the other drivers, let's move it to
> > > a separate function.
> > > 
> > > Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
> > 
> > I'm not really sure why clearing the buffers is a useful thing to do
> > and why we should only do it for YUV and not RGB formats.
> 
> 0 = black for RGB, 0 != black for YUV. The memset was just to make
> things more consistent. But not sure if any test would actually
> fail if we just drop the memset.

Right, I understand that this is required for setting the framebuffer
to black, but I'm just wondering why this has to be done in the first
place.

As far as I know, (RGB) buffers allocated with GEM are not zero-ed by
the allocator, nor by any component in IGT. So I'm wondering why we're
taking the extra time for clearing buffers in the YUV case.

Cheers,

Paul

> > Anyway, since this code was already there and the refactoring
> > definitely makes sense:
> > 
> > Reviewed-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> > 
> > Cheers,
> > 
> > Paul
> > 
> > > ---
> > >  lib/igt_fb.c | 80 +++++++++++++++++++++++++++-------------------------
> > >  1 file changed, 42 insertions(+), 38 deletions(-)
> > > 
> > > diff --git a/lib/igt_fb.c b/lib/igt_fb.c
> > > index 45691fd441d9..d69c3fb2d38d 100644
> > > --- a/lib/igt_fb.c
> > > +++ b/lib/igt_fb.c
> > > @@ -484,6 +484,47 @@ uint64_t igt_fb_tiling_to_mod(uint64_t tiling)
> > >  	}
> > >  }
> > >  
> > > +static void clear_yuv_buffer(struct igt_fb *fb)
> > > +{
> > > +	bool full_range = fb->color_range == IGT_COLOR_YCBCR_FULL_RANGE;
> > > +	void *ptr;
> > > +
> > > +	igt_assert(igt_format_is_yuv(fb->drm_format));
> > > +
> > > +	/* Ensure the framebuffer is preallocated */
> > > +	ptr = igt_fb_map_buffer(fb->fd, fb);
> > > +	igt_assert(*(uint32_t *)ptr == 0);
> > > +
> > > +	switch (fb->drm_format) {
> > > +	case DRM_FORMAT_NV12:
> > > +		memset(ptr + fb->offsets[0],
> > > +		       full_range ? 0x00 : 0x10,
> > > +		       fb->strides[0] * fb->plane_height[0]);
> > > +		memset(ptr + fb->offsets[1],
> > > +		       0x80,
> > > +		       fb->strides[1] * fb->plane_height[1]);
> > > +		break;
> > > +	case DRM_FORMAT_XYUV8888:
> > > +		wmemset(ptr + fb->offsets[0], full_range ? 0x00008080 : 0x00108080,
> > > +			fb->strides[0] * fb->plane_height[0] / sizeof(wchar_t));
> > > +		break;
> > > +	case DRM_FORMAT_YUYV:
> > > +	case DRM_FORMAT_YVYU:
> > > +		wmemset(ptr + fb->offsets[0],
> > > +			full_range ? 0x80008000 : 0x80108010,
> > > +			fb->strides[0] * fb->plane_height[0] / sizeof(wchar_t));
> > > +		break;
> > > +	case DRM_FORMAT_UYVY:
> > > +	case DRM_FORMAT_VYUY:
> > > +		wmemset(ptr + fb->offsets[0],
> > > +			full_range ? 0x00800080 : 0x10801080,
> > > +			fb->strides[0] * fb->plane_height[0] / sizeof(wchar_t));
> > > +		break;
> > > +	}
> > > +
> > > +	igt_fb_unmap_buffer(fb, ptr);
> > > +}
> > > +
> > >  /* helpers to create nice-looking framebuffers */
> > >  static int create_bo_for_fb(struct igt_fb *fb)
> > >  {
> > > @@ -501,50 +542,13 @@ static int create_bo_for_fb(struct igt_fb *fb)
> > >  		fb->is_dumb = false;
> > >  
> > >  		if (is_i915_device(fd)) {
> > > -			void *ptr;
> > > -			bool full_range = fb->color_range == IGT_COLOR_YCBCR_FULL_RANGE;
> > >  
> > >  			fb->gem_handle = gem_create(fd, fb->size);
> > >  
> > >  			gem_set_tiling(fd, fb->gem_handle,
> > >  				       igt_fb_mod_to_tiling(fb->tiling),
> > >  				       fb->strides[0]);
> > > -
> > > -			gem_set_domain(fd, fb->gem_handle,
> > > -				       I915_GEM_DOMAIN_GTT, I915_GEM_DOMAIN_GTT);
> > > -
> > > -			/* Ensure the framebuffer is preallocated */
> > > -			ptr = gem_mmap__gtt(fd, fb->gem_handle,
> > > -					    fb->size, PROT_READ | PROT_WRITE);
> > > -			igt_assert(*(uint32_t *)ptr == 0);
> > > -
> > > -			switch (fb->drm_format) {
> > > -			case DRM_FORMAT_NV12:
> > > -				memset(ptr + fb->offsets[0],
> > > -				       full_range ? 0x00 : 0x10,
> > > -				       fb->strides[0] * fb->plane_height[0]);
> > > -				memset(ptr + fb->offsets[1],
> > > -				       0x80,
> > > -				       fb->strides[1] * fb->plane_height[1]);
> > > -				break;
> > > -			case DRM_FORMAT_XYUV8888:
> > > -				wmemset(ptr + fb->offsets[0], full_range ? 0x00008080 : 0x00108080,
> > > -					fb->strides[0] * fb->plane_height[0] / sizeof(wchar_t));
> > > -				break;
> > > -			case DRM_FORMAT_YUYV:
> > > -			case DRM_FORMAT_YVYU:
> > > -				wmemset(ptr + fb->offsets[0],
> > > -					full_range ? 0x80008000 : 0x80108010,
> > > -					fb->strides[0] * fb->plane_height[0] / sizeof(wchar_t));
> > > -				break;
> > > -			case DRM_FORMAT_UYVY:
> > > -			case DRM_FORMAT_VYUY:
> > > -				wmemset(ptr + fb->offsets[0],
> > > -					full_range ? 0x00800080 : 0x10801080,
> > > -					fb->strides[0] * fb->plane_height[0] / sizeof(wchar_t));
> > > -				break;
> > > -			}
> > > -			gem_munmap(ptr, fb->size);
> > > +			clear_yuv_buffer(fd);
> > >  
> > >  			return fb->gem_handle;
> > >  		} else {
> > -- 
> > Paul Kocialkowski, Bootlin
> > Embedded Linux and kernel engineering
> > https://bootlin.com
On Mon, Jan 14, 2019 at 04:13:02PM +0100, Paul Kocialkowski wrote:
> Hi,
> 
> On Thu, 2019-01-10 at 16:38 +0200, Ville Syrjälä wrote:
> > On Thu, Jan 10, 2019 at 11:10:09AM +0100, Paul Kocialkowski wrote:
> > > On Tue, 2019-01-08 at 16:19 +0100, Maxime Ripard wrote:
> > > > The YUV buffer allocation path for the i915 driver also has some code to
> > > > clear a YUV buffer.
> > > > 
> > > > As that is going to be useful for all the other drivers, let's move it to
> > > > a separate function.
> > > > 
> > > > Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
> > > 
> > > I'm not really sure why clearing the buffers is a useful thing to do
> > > and why we should only do it for YUV and not RGB formats.
> > 
> > 0 = black for RGB, 0 != black for YUV. The memset was just to make
> > things more consistent. But not sure if any test would actually
> > fail if we just drop the memset.
> 
> Right, I understand that this is required for setting the framebuffer
> to black, but I'm just wondering why this has to be done in the first
> place.
> 
> As far as I know, (RGB) buffers allocated with GEM are not zero-ed by
> the allocator,

They are zeroed. You wouldn't want the kernel to leak random
memory contents all over.

> nor by any component in IGT. So I'm wondering why we're
> taking the extra time for clearing buffers in the YUV case.
> 
> Cheers,
> 
> Paul
> 
> > > Anyway, since this code was already there and the refactoring
> > > definitely makes sense:
> > > 
> > > Reviewed-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> > > 
> > > Cheers,
> > > 
> > > Paul
> > > 
> > > > ---
> > > >  lib/igt_fb.c | 80 +++++++++++++++++++++++++++-------------------------
> > > >  1 file changed, 42 insertions(+), 38 deletions(-)
> > > > 
> > > > diff --git a/lib/igt_fb.c b/lib/igt_fb.c
> > > > index 45691fd441d9..d69c3fb2d38d 100644
> > > > --- a/lib/igt_fb.c
> > > > +++ b/lib/igt_fb.c
> > > > @@ -484,6 +484,47 @@ uint64_t igt_fb_tiling_to_mod(uint64_t tiling)
> > > >  	}
> > > >  }
> > > >  
> > > > +static void clear_yuv_buffer(struct igt_fb *fb)
> > > > +{
> > > > +	bool full_range = fb->color_range == IGT_COLOR_YCBCR_FULL_RANGE;
> > > > +	void *ptr;
> > > > +
> > > > +	igt_assert(igt_format_is_yuv(fb->drm_format));
> > > > +
> > > > +	/* Ensure the framebuffer is preallocated */
> > > > +	ptr = igt_fb_map_buffer(fb->fd, fb);
> > > > +	igt_assert(*(uint32_t *)ptr == 0);
> > > > +
> > > > +	switch (fb->drm_format) {
> > > > +	case DRM_FORMAT_NV12:
> > > > +		memset(ptr + fb->offsets[0],
> > > > +		       full_range ? 0x00 : 0x10,
> > > > +		       fb->strides[0] * fb->plane_height[0]);
> > > > +		memset(ptr + fb->offsets[1],
> > > > +		       0x80,
> > > > +		       fb->strides[1] * fb->plane_height[1]);
> > > > +		break;
> > > > +	case DRM_FORMAT_XYUV8888:
> > > > +		wmemset(ptr + fb->offsets[0], full_range ? 0x00008080 : 0x00108080,
> > > > +			fb->strides[0] * fb->plane_height[0] / sizeof(wchar_t));
> > > > +		break;
> > > > +	case DRM_FORMAT_YUYV:
> > > > +	case DRM_FORMAT_YVYU:
> > > > +		wmemset(ptr + fb->offsets[0],
> > > > +			full_range ? 0x80008000 : 0x80108010,
> > > > +			fb->strides[0] * fb->plane_height[0] / sizeof(wchar_t));
> > > > +		break;
> > > > +	case DRM_FORMAT_UYVY:
> > > > +	case DRM_FORMAT_VYUY:
> > > > +		wmemset(ptr + fb->offsets[0],
> > > > +			full_range ? 0x00800080 : 0x10801080,
> > > > +			fb->strides[0] * fb->plane_height[0] / sizeof(wchar_t));
> > > > +		break;
> > > > +	}
> > > > +
> > > > +	igt_fb_unmap_buffer(fb, ptr);
> > > > +}
> > > > +
> > > >  /* helpers to create nice-looking framebuffers */
> > > >  static int create_bo_for_fb(struct igt_fb *fb)
> > > >  {
> > > > @@ -501,50 +542,13 @@ static int create_bo_for_fb(struct igt_fb *fb)
> > > >  		fb->is_dumb = false;
> > > >  
> > > >  		if (is_i915_device(fd)) {
> > > > -			void *ptr;
> > > > -			bool full_range = fb->color_range == IGT_COLOR_YCBCR_FULL_RANGE;
> > > >  
> > > >  			fb->gem_handle = gem_create(fd, fb->size);
> > > >  
> > > >  			gem_set_tiling(fd, fb->gem_handle,
> > > >  				       igt_fb_mod_to_tiling(fb->tiling),
> > > >  				       fb->strides[0]);
> > > > -
> > > > -			gem_set_domain(fd, fb->gem_handle,
> > > > -				       I915_GEM_DOMAIN_GTT, I915_GEM_DOMAIN_GTT);
> > > > -
> > > > -			/* Ensure the framebuffer is preallocated */
> > > > -			ptr = gem_mmap__gtt(fd, fb->gem_handle,
> > > > -					    fb->size, PROT_READ | PROT_WRITE);
> > > > -			igt_assert(*(uint32_t *)ptr == 0);
> > > > -
> > > > -			switch (fb->drm_format) {
> > > > -			case DRM_FORMAT_NV12:
> > > > -				memset(ptr + fb->offsets[0],
> > > > -				       full_range ? 0x00 : 0x10,
> > > > -				       fb->strides[0] * fb->plane_height[0]);
> > > > -				memset(ptr + fb->offsets[1],
> > > > -				       0x80,
> > > > -				       fb->strides[1] * fb->plane_height[1]);
> > > > -				break;
> > > > -			case DRM_FORMAT_XYUV8888:
> > > > -				wmemset(ptr + fb->offsets[0], full_range ? 0x00008080 : 0x00108080,
> > > > -					fb->strides[0] * fb->plane_height[0] / sizeof(wchar_t));
> > > > -				break;
> > > > -			case DRM_FORMAT_YUYV:
> > > > -			case DRM_FORMAT_YVYU:
> > > > -				wmemset(ptr + fb->offsets[0],
> > > > -					full_range ? 0x80008000 : 0x80108010,
> > > > -					fb->strides[0] * fb->plane_height[0] / sizeof(wchar_t));
> > > > -				break;
> > > > -			case DRM_FORMAT_UYVY:
> > > > -			case DRM_FORMAT_VYUY:
> > > > -				wmemset(ptr + fb->offsets[0],
> > > > -					full_range ? 0x00800080 : 0x10801080,
> > > > -					fb->strides[0] * fb->plane_height[0] / sizeof(wchar_t));
> > > > -				break;
> > > > -			}
> > > > -			gem_munmap(ptr, fb->size);
> > > > +			clear_yuv_buffer(fd);
> > > >  
> > > >  			return fb->gem_handle;
> > > >  		} else {
> > > -- 
> > > Paul Kocialkowski, Bootlin
> > > Embedded Linux and kernel engineering
> > > https://bootlin.com
> -- 
> Paul Kocialkowski, Bootlin
> Embedded Linux and kernel engineering
> https://bootlin.com
Hi,

On Mon, 2019-01-14 at 18:40 +0200, Ville Syrjälä wrote:
> On Mon, Jan 14, 2019 at 04:13:02PM +0100, Paul Kocialkowski wrote:
> > Hi,
> > 
> > On Thu, 2019-01-10 at 16:38 +0200, Ville Syrjälä wrote:
> > > On Thu, Jan 10, 2019 at 11:10:09AM +0100, Paul Kocialkowski wrote:
> > > > On Tue, 2019-01-08 at 16:19 +0100, Maxime Ripard wrote:
> > > > > The YUV buffer allocation path for the i915 driver also has some code to
> > > > > clear a YUV buffer.
> > > > > 
> > > > > As that is going to be useful for all the other drivers, let's move it to
> > > > > a separate function.
> > > > > 
> > > > > Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
> > > > 
> > > > I'm not really sure why clearing the buffers is a useful thing to do
> > > > and why we should only do it for YUV and not RGB formats.
> > > 
> > > 0 = black for RGB, 0 != black for YUV. The memset was just to make
> > > things more consistent. But not sure if any test would actually
> > > fail if we just drop the memset.
> > 
> > Right, I understand that this is required for setting the framebuffer
> > to black, but I'm just wondering why this has to be done in the first
> > place.
> > 
> > As far as I know, (RGB) buffers allocated with GEM are not zero-ed by
> > the allocator,
> 
> They are zeroed. You wouldn't want the kernel to leak random
> memory contents all over.

Oh my mistake! It definitely makes sense that the kernel does that.
Let's keep things consistent and have all buffers initialized with
black then.

Cheers,

Paul

> > nor by any component in IGT. So I'm wondering why we're
> > taking the extra time for clearing buffers in the YUV case.
> > 
> > Cheers,
> > 
> > Paul
> > 
> > > > Anyway, since this code was already there and the refactoring
> > > > definitely makes sense:
> > > > 
> > > > Reviewed-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> > > > 
> > > > Cheers,
> > > > 
> > > > Paul
> > > > 
> > > > > ---
> > > > >  lib/igt_fb.c | 80 +++++++++++++++++++++++++++-------------------------
> > > > >  1 file changed, 42 insertions(+), 38 deletions(-)
> > > > > 
> > > > > diff --git a/lib/igt_fb.c b/lib/igt_fb.c
> > > > > index 45691fd441d9..d69c3fb2d38d 100644
> > > > > --- a/lib/igt_fb.c
> > > > > +++ b/lib/igt_fb.c
> > > > > @@ -484,6 +484,47 @@ uint64_t igt_fb_tiling_to_mod(uint64_t tiling)
> > > > >  	}
> > > > >  }
> > > > >  
> > > > > +static void clear_yuv_buffer(struct igt_fb *fb)
> > > > > +{
> > > > > +	bool full_range = fb->color_range == IGT_COLOR_YCBCR_FULL_RANGE;
> > > > > +	void *ptr;
> > > > > +
> > > > > +	igt_assert(igt_format_is_yuv(fb->drm_format));
> > > > > +
> > > > > +	/* Ensure the framebuffer is preallocated */
> > > > > +	ptr = igt_fb_map_buffer(fb->fd, fb);
> > > > > +	igt_assert(*(uint32_t *)ptr == 0);
> > > > > +
> > > > > +	switch (fb->drm_format) {
> > > > > +	case DRM_FORMAT_NV12:
> > > > > +		memset(ptr + fb->offsets[0],
> > > > > +		       full_range ? 0x00 : 0x10,
> > > > > +		       fb->strides[0] * fb->plane_height[0]);
> > > > > +		memset(ptr + fb->offsets[1],
> > > > > +		       0x80,
> > > > > +		       fb->strides[1] * fb->plane_height[1]);
> > > > > +		break;
> > > > > +	case DRM_FORMAT_XYUV8888:
> > > > > +		wmemset(ptr + fb->offsets[0], full_range ? 0x00008080 : 0x00108080,
> > > > > +			fb->strides[0] * fb->plane_height[0] / sizeof(wchar_t));
> > > > > +		break;
> > > > > +	case DRM_FORMAT_YUYV:
> > > > > +	case DRM_FORMAT_YVYU:
> > > > > +		wmemset(ptr + fb->offsets[0],
> > > > > +			full_range ? 0x80008000 : 0x80108010,
> > > > > +			fb->strides[0] * fb->plane_height[0] / sizeof(wchar_t));
> > > > > +		break;
> > > > > +	case DRM_FORMAT_UYVY:
> > > > > +	case DRM_FORMAT_VYUY:
> > > > > +		wmemset(ptr + fb->offsets[0],
> > > > > +			full_range ? 0x00800080 : 0x10801080,
> > > > > +			fb->strides[0] * fb->plane_height[0] / sizeof(wchar_t));
> > > > > +		break;
> > > > > +	}
> > > > > +
> > > > > +	igt_fb_unmap_buffer(fb, ptr);
> > > > > +}
> > > > > +
> > > > >  /* helpers to create nice-looking framebuffers */
> > > > >  static int create_bo_for_fb(struct igt_fb *fb)
> > > > >  {
> > > > > @@ -501,50 +542,13 @@ static int create_bo_for_fb(struct igt_fb *fb)
> > > > >  		fb->is_dumb = false;
> > > > >  
> > > > >  		if (is_i915_device(fd)) {
> > > > > -			void *ptr;
> > > > > -			bool full_range = fb->color_range == IGT_COLOR_YCBCR_FULL_RANGE;
> > > > >  
> > > > >  			fb->gem_handle = gem_create(fd, fb->size);
> > > > >  
> > > > >  			gem_set_tiling(fd, fb->gem_handle,
> > > > >  				       igt_fb_mod_to_tiling(fb->tiling),
> > > > >  				       fb->strides[0]);
> > > > > -
> > > > > -			gem_set_domain(fd, fb->gem_handle,
> > > > > -				       I915_GEM_DOMAIN_GTT, I915_GEM_DOMAIN_GTT);
> > > > > -
> > > > > -			/* Ensure the framebuffer is preallocated */
> > > > > -			ptr = gem_mmap__gtt(fd, fb->gem_handle,
> > > > > -					    fb->size, PROT_READ | PROT_WRITE);
> > > > > -			igt_assert(*(uint32_t *)ptr == 0);
> > > > > -
> > > > > -			switch (fb->drm_format) {
> > > > > -			case DRM_FORMAT_NV12:
> > > > > -				memset(ptr + fb->offsets[0],
> > > > > -				       full_range ? 0x00 : 0x10,
> > > > > -				       fb->strides[0] * fb->plane_height[0]);
> > > > > -				memset(ptr + fb->offsets[1],
> > > > > -				       0x80,
> > > > > -				       fb->strides[1] * fb->plane_height[1]);
> > > > > -				break;
> > > > > -			case DRM_FORMAT_XYUV8888:
> > > > > -				wmemset(ptr + fb->offsets[0], full_range ? 0x00008080 : 0x00108080,
> > > > > -					fb->strides[0] * fb->plane_height[0] / sizeof(wchar_t));
> > > > > -				break;
> > > > > -			case DRM_FORMAT_YUYV:
> > > > > -			case DRM_FORMAT_YVYU:
> > > > > -				wmemset(ptr + fb->offsets[0],
> > > > > -					full_range ? 0x80008000 : 0x80108010,
> > > > > -					fb->strides[0] * fb->plane_height[0] / sizeof(wchar_t));
> > > > > -				break;
> > > > > -			case DRM_FORMAT_UYVY:
> > > > > -			case DRM_FORMAT_VYUY:
> > > > > -				wmemset(ptr + fb->offsets[0],
> > > > > -					full_range ? 0x00800080 : 0x10801080,
> > > > > -					fb->strides[0] * fb->plane_height[0] / sizeof(wchar_t));
> > > > > -				break;
> > > > > -			}
> > > > > -			gem_munmap(ptr, fb->size);
> > > > > +			clear_yuv_buffer(fd);
> > > > >  
> > > > >  			return fb->gem_handle;
> > > > >  		} else {
> > > > -- 
> > > > Paul Kocialkowski, Bootlin
> > > > Embedded Linux and kernel engineering
> > > > https://bootlin.com
> > -- 
> > Paul Kocialkowski, Bootlin
> > Embedded Linux and kernel engineering
> > https://bootlin.com