[igt-dev] [PATCH i-g-t 2/5] lib/gen6_render.h: Use gen6 definitions where it is possible

[Katarzyna Dec]

On Mon, Jun 11, 2018 at 01:46:26PM +0100, Kalamarz, Lukasz wrote:
> On Mon, 2018-06-11 at 09:44 +0200, Katarzyna Dec wrote:
> > On Fri, Jun 08, 2018 at 01:38:52PM +0200, Lukasz Kalamarz wrote:
> > I think lib/gen6_render.h header is not for this patch. 
> > And with adding new header title needs to be changed I guess.
> > 
> > (this header confused my when I started to do a review on Fri - I
> > thought
> > that changes are made in gen6_render :) )
> > 
> 
> My bad, will fix that.
> 
> > I would also changed commit msg, maybe sth like this:
> > > As long as it is applicable, We should use in our libs
> > > definitions from oldest gen if it is possible.
> > 
> > When using genX_render definitions, we should use the oldest
> > definition it
> > is possible. 
> > 
> 
> Will fix that in next version.
> 
> > > This patch
> > > reuse gen6 definitons if registers/fields/shifts that were
> > > reintroduced in other genX_render headers.
> > > 
> > > Signed-off-by: Lukasz Kalamarz <lukasz.kalamarz at intel.com>
> > > Cc: Katarzyna Dec <katarzyna.dec at intel.com>
> > > Cc: Antonio Argenziano <antonio.argenziano at intel.com>
> > > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
> > > Cc: Ewelina Musial <ewelina.musial at intel.com>
> > > ---
> > >  lib/rendercopy_gen7.c                         | 108 +++++++++++++-
> > > ------------
> > >  lib/rendercopy_gen8.c                         |   4 +-
> > >  lib/rendercopy_gen9.c                         |   4 +-
> > >  tools/null_state_gen/intel_renderstate_gen7.c | 108 +++++++++++++-
> > > ------------
> > >  tools/null_state_gen/intel_renderstate_gen8.c |  10 +--
> > >  tools/null_state_gen/intel_renderstate_gen9.c |  10 +--
> > >  6 files changed, 122 insertions(+), 122 deletions(-)
> > > 
> > > diff --git a/lib/rendercopy_gen7.c b/lib/rendercopy_gen7.c
> > > index 82e33288..bdcf3c7b 100644
> > > --- a/lib/rendercopy_gen7.c
> > > +++ b/lib/rendercopy_gen7.c
> > > @@ -101,35 +101,35 @@ gen7_bind_buf(struct intel_batchbuffer
> > > *batch,
> > >  static void
> > >  gen7_emit_vertex_elements(struct intel_batchbuffer *batch)
> > >  {
> > > -	OUT_BATCH(GEN7_3DSTATE_VERTEX_ELEMENTS |
> > > +	OUT_BATCH(GEN6_3DSTATE_VERTEX_ELEMENTS |
> > >  		  ((2 * (1 + 2)) + 1 - 2));
> > >  
> > > -	OUT_BATCH(0 << GEN7_VE0_VERTEX_BUFFER_INDEX_SHIFT |
> > > GEN7_VE0_VALID |
> > > -		  SURFACEFORMAT_R32G32B32A32_FLOAT <<
> > > GEN7_VE0_FORMAT_SHIFT |
> > > -		  0 << GEN7_VE0_OFFSET_SHIFT);
> > > +	OUT_BATCH(0 << VE0_VERTEX_BUFFER_INDEX_SHIFT | VE0_VALID |
> > > +		  SURFACEFORMAT_R32G32B32A32_FLOAT <<
> > > VE0_FORMAT_SHIFT |
> > > +		  0 << VE0_OFFSET_SHIFT);
> > 
> > Why GENX_prefix was removed from SURFACEFORMAT_R32G32B32A32_FLOAT (in
> > already
> > merged patches), but this prefix is still in
> > e.g.GEN6_3DSTATE_VERTEX_ELEMENTS?
> > This is only a question for a reason, not a suggestion :)
> 
> Surface formats were not changing, but some registers/fields/shifts
> changed their definitions across gens. This is why I believe we should
> stay with this naming convention. 
>
Fair enough,
Kasia :)

> > 
> > Kasia :)
> >