Re: [igt-dev] [PATCH i-g-t v5 3/3] i915/gem_render_copy.c: Simplify code by switch to rendercopy bufmgr

["Kempczynski, Zbigniew" <zbigniew.kempczynski@intel.com>]

On Wed, 2020-01-08 at 11:12 +0000, Chris Wilson wrote:
> Quoting Zbigniew Kempczyński (2020-01-08 07:54:11)
> > Switch to rendercopy bufmgr to simplify working with tiled surfaces.
> > 
> > Signed-off-by: Zbigniew Kempczyński <zbigniew.kempczynski@intel.com>
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Katarzyna Dec <katarzyna.dec@intel.com>
> > ---
> >  lib/intel_batchbuffer.c      |  47 +++++
> >  lib/intel_batchbuffer.h      |   2 +
> >  tests/i915/gem_render_copy.c | 370 +++++++----------------------------
> >  3 files changed, 122 insertions(+), 297 deletions(-)
> > 
> > diff --git a/lib/intel_batchbuffer.c b/lib/intel_batchbuffer.c
> > index 3dc89024..c23e5e48 100644
> > --- a/lib/intel_batchbuffer.c
> > +++ b/lib/intel_batchbuffer.c
> > @@ -43,6 +43,7 @@
> >  #include "ioctl_wrappers.h"
> >  #include "media_spin.h"
> >  #include "gpgpu_fill.h"
> > +#include "igt_aux.h"
> >  
> >  #include <i915_drm.h>
> >  
> > @@ -529,6 +530,52 @@ unsigned igt_buf_height(const struct igt_buf *buf)
> >         return buf->surface[0].size/buf->surface[0].stride;
> >  }
> >  
> > +/**
> > + * igt_buf_ccs_width:
> > + * @buf: the i-g-t buffer object
> > + * @gen: device generation
> > + *
> > + * Computes the width of ccs buffer data.
> > + *
> > + * Returns:
> > + * The width of the ccs buffer data.
> > + */
> > +unsigned int igt_buf_ccs_width(int gen, const struct igt_buf *buf)
> > +{
> > +       /*
> > +        * GEN12+: The CCS unit size is 64 bytes mapping 4 main surface
> > +        * tiles. Thus the width of the CCS unit is 4*32=128 pixels on the
> > +        * main surface.
> > +        */
> > +       if (gen >= 12)
> > +               return DIV_ROUND_UP(igt_buf_width(buf), 128) * 64;
> 
> This should not be igt_, since it is not vendor neutral.

Agree. I will find better name and resubmit in v6.

> 
> Atm we have
> 	intel_: uses libdrm_intel (deprecated)
> 	gem_: uses DRM_I915_GEM ioctls (bad prefix, needs to be i915_gem?)
> 	rendercopy_: sounds generic, could be, but isn't really atm :)
> 	igt_kms_: mostly generic, at least tries to support equivalent
> 	          paths on different vendors
> 
> I would probably also change igt_buf to igt_image for the
> implication of it supporting multidimension [tiled] surfaces, but igt_
> is a good prefix for a generic handle + surface info.

igt_buf has long story I don't want to change in this patch series. 
If you think we should rename this structure name it should be
next patch series targeted to that.

> I'm tempted to move all the mediacopy, gpgpucopy, rendercopy under
> lib/gpucopy/ and hope one day we have a generic interface, but really
> not sure if that's ever going to be required. (When we get to cross
> vendor behaviour, we're better off looking at Vulkan or some other
> lowlevel generic api, at which point we are clearly outside the scope of
> ioctl ABI testing.) So, I'd suggest moving these to lib/i915/gpucopy.
> -Chris

I agree but as I've mentioned I think it shouldn't be part of this patch
series.

--
Zbigniew
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev