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

From: "Kalamarz, Lukasz" <lukasz.kalamarz@intel.com>
To: "Dec, Katarzyna" <katarzyna.dec@intel.com>
Cc: "igt-dev@lists.freedesktop.org" <igt-dev@lists.freedesktop.org>
Subject: Re: [igt-dev] [PATCH i-g-t 2/5] lib/gen6_render.h: Use gen6 definitions where it is possible
Date: Mon, 11 Jun 2018 12:46:26 +0000	[thread overview]
Message-ID: <1528721184.4203.57.camel@intel.com> (raw)
In-Reply-To: <20180611074434.GF8119@kdec5-desk.ger.corp.intel.com>

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@intel.com>
> > Cc: Katarzyna Dec <katarzyna.dec@intel.com>
> > Cc: Antonio Argenziano <antonio.argenziano@intel.com>
> > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> > Cc: Ewelina Musial <ewelina.musial@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. 

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