[Mesa-dev] [PATCH] i965: implement (un)mapImage

[Rafael Antognolli]

On Fri, Nov 10, 2017 at 10:46:03AM +0000, Julien Isorce wrote:
> Thx for the suggestions.
> 
> Anyone familiar with _mesa_get_format_block_size and _mesa_get_format_bytes
> wants to review this patch ? 
> 
> On 9 November 2017 at 17:10, Eric Engestrom <eric.engestrom at imgtec.com> wrote:
> 
>     On Thursday, 2017-11-09 17:03:13 +0000, Julien Isorce wrote:
>     > v2: add early return if (flag & MAP_INTERNAL_MASK)
>     > v3: take input rect into account and test with kmscube and piglit.
>     > v4: handle wraparound and bo reference.

Just nitpick, but in case you need to respin a new version of this
patch, please consider adding the changelog (v2-n) after the commit
description. See other commits for some examples.

Rafael

>     > Already implemented for Gallium drivers.
>     >
>     > Useful for gbm_bo_(un)map.
>     >
>     > Tests:
>     >   By porting wayland/weston/clients/simple-dmabuf-drm.c to GBM.
>     >   kmscube --mode=rgba
>     >   kmscube --mode=nv12-1img
>     >   kmscube --mode=nv12-2img
>     >   piglit ext_image_dma_buf_import-refcount -auto
>     >   piglit ext_image_dma_buf_import-transcode-nv12-as-r8-gr88 -auto
>     >   piglit ext_image_dma_buf_import-sample_rgb -fmt=AR24 -alpha-one -auto
>     >   piglit ext_image_dma_buf_import-sample_rgb -fmt=XR24 -auto
>     >   piglit ext_image_dma_buf_import-sample_yuv -fmt=NV12 -auto
>     >   piglit ext_image_dma_buf_import-sample_yuv -fmt=YU12 -auto
>     >   piglit ext_image_dma_buf_import-sample_yuv -fmt=YV12 -auto
>     >
>     > Signed-off-by: Julien Isorce <jisorce at oblong.com>
>     > ---
>     >  src/mesa/drivers/dri/i965/intel_screen.c | 75
>     +++++++++++++++++++++++++++++++-
>     >  1 file changed, 73 insertions(+), 2 deletions(-)
>     >
>     > diff --git a/src/mesa/drivers/dri/i965/intel_screen.c b/src/mesa/drivers/
>     dri/i965/intel_screen.c
>     > index cdc36ad..6074ee8 100644
>     > --- a/src/mesa/drivers/dri/i965/intel_screen.c
>     > +++ b/src/mesa/drivers/dri/i965/intel_screen.c
>     > @@ -755,6 +755,77 @@ intel_create_image(__DRIscreen *dri_screen,
>     >                                 loaderPrivate);
>     >  }
>     >
>     > +static void *
>     > +intel_map_image(__DRIcontext *context, __DRIimage *image,
>     > +                int x0, int y0, int width, int height,
>     > +                unsigned int flags, int *stride, void **map_info)
>     > +{
>     > +   struct brw_context *brw = NULL;
>     > +   struct brw_bo *bo = NULL;
>     > +   void *raw_data = NULL;
>     > +
>     > +   if (!context || !image || !stride || !map_info || *map_info)
>     > +      return NULL;
>     > +
>     > +   if (x0 < 0 || x0 > image->width || width > image->width - x0)
>     > +      return NULL;
>     > +
>     > +   if (y0 < 0 || y0 > image->height || height > image->height - y0)
>     > +        return NULL;
>     > +
>     > +   if (flags & MAP_INTERNAL_MASK)
>     > +      return NULL;
>     > +
>     > +   brw = context->driverPrivate;
>     > +   bo = image->bo;
>     > +
>     > +   assert(brw);
>     > +   assert(bo);
>     > +
>     > +   brw_bo_reference(bo);
>     > +
>     > +   /* DRI flags and GL_MAP.*_BIT flags are the same, so just pass them
>     on. */
>     > +   raw_data = brw_bo_map(brw, bo, flags);
>     > +
>     > +   if (raw_data) {
>     > +      GLuint pix_w = 1;
>     > +      GLuint pix_h = 1;
>     > +      GLint pix_bytes = 1;
>     > +
>     > +      *map_info = raw_data;
>     > +      *stride = image->pitch;
>     > +
>     > +      _mesa_get_format_block_size(image->format, &pix_w, &pix_h);
>     > +      pix_bytes = _mesa_get_format_bytes(image->format);
>     > +
>     > +      assert(pix_w);
>     > +      assert(pix_h);
>     > +      assert(pix_bytes > 0);
>     > +
>     > +      raw_data += ((x0 / pix_w) * pix_bytes) + (y0 / pix_h) * image->
>     pitch;
>     > +   } else {
>     > +      brw_bo_unreference(bo);
>     > +   }
> 
>     Feels really nit-picky, so don't send a new rev just for that, but:
> 
>        if (!raw_data) {
>           brw_bo_unreference(bo);
>           return NULL;
>        }
> 
>        /* code from the `if (raw_data)` branch */
> 
>     that way you don't have to carry around the indentation :)
> 
>     > +
>     > +   return raw_data;
>     > +}
>     > +
>     > +static void
>     > +intel_unmap_image(__DRIcontext *context, __DRIimage *image, void
>     *map_info)
>     > +{
>     > +   struct brw_bo *bo = NULL;
>     > +
>     > +   if (!context || !image || !map_info)
>     > +      return;
>     > +
>     > +   bo = image->bo;
>     > +
>     > +   assert(bo);
>     > +
>     > +   brw_bo_unmap(bo);
>     > +   brw_bo_unreference(bo);
>     > +}
>     > +
>     >  static __DRIimage *
>     >  intel_create_image_with_modifiers(__DRIscreen *dri_screen,
>     >                                    int width, int height, int format,
>     > @@ -1305,8 +1376,8 @@ static const __DRIimageExtension
>     intelImageExtension = {
>     >      .createImageFromDmaBufs             = intel_create_image_from_dma_
>     bufs,
>     >      .blitImage                          = NULL,
>     >      .getCapabilities                    = NULL,
>     > -    .mapImage                           = NULL,
>     > -    .unmapImage                         = NULL,
>     > +    .mapImage                           = intel_map_image,
>     > +    .unmapImage                         = intel_unmap_image,
>     >      .createImageWithModifiers           = intel_create_image_with_
>     modifiers,
>     >      .createImageFromDmaBufs2            = intel_create_image_from_dma_
>     bufs2,
>     >      .queryDmaBufFormats                 = intel_query_dma_buf_formats,
>     > --
>     > 2.7.4
>     >
> 
> 

> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev