[igt-dev] [PATCH i-g-t v2 04/13] igt: fb: Move i915 YUV buffer clearing code to a function
Paul Kocialkowski
paul.kocialkowski at bootlin.com
Tue Jan 15 16:15:58 UTC 2019
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 at 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 at 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
--
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com
More information about the igt-dev
mailing list