Re: [igt-dev] [PATCH i-g-t] lib/rendercopy: Add gen4/5 rendercopy

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Wed, Jun 13, 2018 at 10:35:56AM +0000, Kalamarz, Lukasz wrote:
> On Mon, 2018-06-11 at 19:14 +0300, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Add rendercopy implementation for gen4/5. Basic structure
> > copied from the gen6 implementation, 
> 
> After refactoring some part of rendercopy libs I don't like that
> sentence :(
> 
> > and the gen4/5 specific
> > bits were mostly lifted from sna.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  lib/Makefile.sources    |   2 +
> >  lib/gen4_render.h       | 628
> > ++++++++++++++++++++++++++++++++++++++++++
> >  lib/intel_batchbuffer.c |   2 +
> >  lib/meson.build         |   1 +
> >  lib/rendercopy.h        |   5 +
> >  lib/rendercopy_gen4.c   | 704
> > ++++++++++++++++++++++++++++++++++++++++++++++++
> >  6 files changed, 1342 insertions(+)
> >  create mode 100644 lib/gen4_render.h
> >  create mode 100644 lib/rendercopy_gen4.c
> > 
> > diff --git a/lib/Makefile.sources b/lib/Makefile.sources
> > index 042c1d3bb44a..e0ebd02c1661 100644
> > --- a/lib/Makefile.sources
> > +++ b/lib/Makefile.sources
> > @@ -71,10 +71,12 @@ lib_source_list =	 	\
> >  	gen8_media.h            \
> >  	rendercopy_i915.c	\
> >  	rendercopy_i830.c	\
> > +	gen4_render.h		\
> >  	gen6_render.h		\
> >  	gen7_render.h		\
> >  	gen8_render.h		\
> >  	gen9_render.h		\
> > +	rendercopy_gen4.c	\
> >  	rendercopy_gen6.c	\
> >  	rendercopy_gen7.c	\
> >  	rendercopy_gen8.c	\
> > diff --git a/lib/gen4_render.h b/lib/gen4_render.h
> > new file mode 100644
> > index 000000000000..ab1158e3c6d2
> > --- /dev/null
> > +++ b/lib/gen4_render.h
> 
> With having in mind refactoring of genX_render libs introduced in patch
> series: https://patchwork.freedesktop.org/series/44624/ Could You check
> if registers defined here with GEN4/5 prefix are reimplmented in
> gen6_render? If so, then maybe it will be good idea to modify those
> definitions and not add more duplicated definitions?

There are many things that didn't change from gen4 onwards. I think the
correct option would be to remove the duplicate definitions from gen6+
and inherit the gen4 stuff.

> 
> > @@ -0,0 +1,628 @@
> > +#ifndef GEN4_RENDER_H
> > +#define GEN4_RENDER_H
> 
> <snip>
> 
> > diff --git a/lib/rendercopy_gen4.c b/lib/rendercopy_gen4.c
> > new file mode 100644
> > index 000000000000..acd4be8de9da
> > --- /dev/null
> > +++ b/lib/rendercopy_gen4.c
> > @@ -0,0 +1,704 @@
> > +#include "rendercopy.h"
> > +#include "intel_chipset.h"
> > +#include "gen4_render.h"
> > +#include "surfaceformat.h"
> > +
> > +#include <assert.h>
> > +
> > +#define VERTEX_SIZE (3*4)
> > +
> > +#define URB_VS_ENTRY_SIZE	1
> > +#define URB_GS_ENTRY_SIZE	0
> > +#define URB_CL_ENTRY_SIZE	0
> > +#define URB_SF_ENTRY_SIZE	2
> > +#define URB_CS_ENTRY_SIZE	1
> > +
> > +#define GEN4_GRF_BLOCKS(nreg) (((nreg) + 15) / 16 - 1)
> > +#define SF_KERNEL_NUM_GRF 16
> > +#define PS_KERNEL_NUM_GRF 32
> > +
> > +static const uint32_t gen4_sf_kernel_nomask[][4] = {
> > +	{ 0x00400031, 0x20c01fbd, 0x0069002c, 0x01110001 },
> > +	{ 0x00600001, 0x206003be, 0x00690060, 0x00000000 },
> > +	{ 0x00600040, 0x20e077bd, 0x00690080, 0x006940a0 },
> > +	{ 0x00600041, 0x202077be, 0x008d00e0, 0x000000c0 },
> > +	{ 0x00600040, 0x20e077bd, 0x006900a0, 0x00694060 },
> > +	{ 0x00600041, 0x204077be, 0x008d00e0, 0x000000c8 },
> > +	{ 0x00600031, 0x20001fbc, 0x008d0000, 0x8640c800 },
> > +};
> > +
> > +static const uint32_t gen5_sf_kernel_nomask[][4] = {
> > +	{ 0x00400031, 0x20c01fbd, 0x1069002c, 0x02100001 },
> > +	{ 0x00600001, 0x206003be, 0x00690060, 0x00000000 },
> > +	{ 0x00600040, 0x20e077bd, 0x00690080, 0x006940a0 },
> > +	{ 0x00600041, 0x202077be, 0x008d00e0, 0x000000c0 },
> > +	{ 0x00600040, 0x20e077bd, 0x006900a0, 0x00694060 },
> > +	{ 0x00600041, 0x204077be, 0x008d00e0, 0x000000c8 },
> > +	{ 0x00600031, 0x20001fbc, 0x648d0000, 0x8808c800 },
> > +};
> > +
> > +static const uint32_t gen4_ps_kernel_nomask_affine[][4] = {
> > +	{ 0x00800040, 0x23c06d29, 0x00480028, 0x10101010 },
> > +	{ 0x00800040, 0x23806d29, 0x0048002a, 0x11001100 },
> > +	{ 0x00802040, 0x2100753d, 0x008d03c0, 0x00004020 },
> > +	{ 0x00802040, 0x2140753d, 0x008d0380, 0x00004024 },
> > +	{ 0x00802059, 0x200077bc, 0x00000060, 0x008d0100 },
> > +	{ 0x00802048, 0x204077be, 0x00000064, 0x008d0140 },
> > +	{ 0x00802059, 0x200077bc, 0x00000070, 0x008d0100 },
> > +	{ 0x00802048, 0x208077be, 0x00000074, 0x008d0140 },
> > +	{ 0x00600201, 0x20200022, 0x008d0000, 0x00000000 },
> > +	{ 0x00000201, 0x20280062, 0x00000000, 0x00000000 },
> > +	{ 0x01800031, 0x21801d09, 0x008d0000, 0x02580001 },
> > +	{ 0x00600001, 0x204003be, 0x008d0180, 0x00000000 },
> > +	{ 0x00601001, 0x20c003be, 0x008d01a0, 0x00000000 },
> > +	{ 0x00600001, 0x206003be, 0x008d01c0, 0x00000000 },
> > +	{ 0x00601001, 0x20e003be, 0x008d01e0, 0x00000000 },
> > +	{ 0x00600001, 0x208003be, 0x008d0200, 0x00000000 },
> > +	{ 0x00601001, 0x210003be, 0x008d0220, 0x00000000 },
> > +	{ 0x00600001, 0x20a003be, 0x008d0240, 0x00000000 },
> > +	{ 0x00601001, 0x212003be, 0x008d0260, 0x00000000 },
> > +	{ 0x00600201, 0x202003be, 0x008d0020, 0x00000000 },
> > +	{ 0x00800031, 0x20001d28, 0x008d0000, 0x85a04800 },
> > +};
> > +
> > +static const uint32_t gen5_ps_kernel_nomask_affine[][4] = {
> > +	{ 0x00800040, 0x23c06d29, 0x00480028, 0x10101010 },
> > +	{ 0x00800040, 0x23806d29, 0x0048002a, 0x11001100 },
> > +	{ 0x00802040, 0x2100753d, 0x008d03c0, 0x00004020 },
> > +	{ 0x00802040, 0x2140753d, 0x008d0380, 0x00004024 },
> > +	{ 0x00802059, 0x200077bc, 0x00000060, 0x008d0100 },
> > +	{ 0x00802048, 0x204077be, 0x00000064, 0x008d0140 },
> > +	{ 0x00802059, 0x200077bc, 0x00000070, 0x008d0100 },
> > +	{ 0x00802048, 0x208077be, 0x00000074, 0x008d0140 },
> > +	{ 0x01800031, 0x21801fa9, 0x208d0000, 0x0a8a0001 },
> > +	{ 0x00802001, 0x304003be, 0x008d0180, 0x00000000 },
> > +	{ 0x00802001, 0x306003be, 0x008d01c0, 0x00000000 },
> > +	{ 0x00802001, 0x308003be, 0x008d0200, 0x00000000 },
> > +	{ 0x00802001, 0x30a003be, 0x008d0240, 0x00000000 },
> > +	{ 0x00600201, 0x202003be, 0x008d0020, 0x00000000 },
> > +	{ 0x00800031, 0x20001d28, 0x548d0000, 0x94084800 },
> > +};
> > +
> > +static uint32_t
> > +batch_used(struct intel_batchbuffer *batch)
> > +{
> > +	return batch->ptr - batch->buffer;
> > +}
> > +
> > +static uint32_t
> > +batch_round_upto(struct intel_batchbuffer *batch, uint32_t divisor)
> > +{
> > +	uint32_t offset = batch_used(batch);
> > +	offset = (offset + divisor - 1) / divisor * divisor;
> > +	batch->ptr = batch->buffer + offset;
> > +	return offset;
> > +}
> 
> With the usage of the same methods in two libs (gen4 and gen6), maybe
> it will be worth to move those functions to intel_batchbuffer (like it
> was done with previous copy/pasted functions)?

Probably a better approach is to change gen4/6 to create the
vertex buffer the same way as gen7+.

-- 
Ville Syrjälä
Intel
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev