[igt-dev] [PATCH i-g-t 1/7] lib/rendercopy: Add support for Yf/Ys tiling to gen9 rendercopy

[Katarzyna Dec]

On Tue, Feb 26, 2019 at 09:00:33PM +0000, Pandiyan, Dhinakaran wrote:
> On Mon, 2019-02-25 at 14:39 +0100, Katarzyna Dec wrote:
> > On Thu, Feb 21, 2019 at 06:41:03AM -0800, Dhinakaran Pandiyan wrote:
> > > From: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > > 
> > > Set up the surface state accordingly to support Yf/Ys tiling.
> > > 
> > > From DK:
> > >  Rebase.
> > > 
> > > Cc: Lukasz Kalamarz <lukasz.kalamarz at intel.com>
> > > Cc: Katarzyna Dec <katarzyna.dec at intel.com>
> > > Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan at intel.com>
> > > ---
> > >  lib/gen8_render.h     | 6 ++++--
> > >  lib/rendercopy_gen9.c | 8 +++++++-
> > >  2 files changed, 11 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/lib/gen8_render.h b/lib/gen8_render.h
> > > index c62047d8..372c5267 100644
> > > --- a/lib/gen8_render.h
> > > +++ b/lib/gen8_render.h
> > 
> > I do not like the idea of adding gen9+ things to gen8 library. We
> > have
> > gen9_render.h already, maybe you could add this stuct there?
> 
> That can be done, but looking at the git log history, I see that the
> convention has been to be reuse older definitions whenever possible.
> And in this case, the definitions do not conflict with the existing
> gen-8 bits.
> 
> Also, given that there is already a history of adding gen9+ definitions
> (AUX_CCS_E) in this struct, I'm inclined to keep the patch as is.
> 
> If you insist, I think we should move other gen-9 definitions in this
> struct as well and do it as a separate patch.
> 
> -DK

There was a time when together with Lukasz K we were doing refactoring of
rendercopy/gpgpu_fill and media_fill. Gpgpu_fill and media_fill changes are
done, but I think that Lukasz did not finish with rendercopy for some reasons.
I'd preffer to have this change in gen9 file together with moving
' # define GEN9_3DSTATE_MULTISAMPLE_NUMSAMPLES_16                 (4 << 1)'
there. As I see it is the only change gen9 related in gen8 for now.
Let's try to clean up. It will be easier to maintain and understand to others.

Thanks,
Kasia
> 
> > Kasia
> > > @@ -121,9 +121,11 @@ struct gen8_surface_state
> > >  	struct {
> > >  		uint32_t mip_count:4;
> > >  		uint32_t min_lod:4;
> > > -		uint32_t pad3:6;
> > > +		uint32_t mip_tail_start_lod:4; /* gen9+ */
> > > +		uint32_t pad3:2;
> > >  		uint32_t coherency_type:1;
> > > -		uint32_t pad2:5;
> > > +		uint32_t pad2:3;
> > > +		uint32_t trmode:2; /* gen9+ */
> > >  		uint32_t ewa_disable_for_cube:1;
> > >  		uint32_t y_offset:3;
> > >  		uint32_t pad0:1;
> > > diff --git a/lib/rendercopy_gen9.c b/lib/rendercopy_gen9.c
> > > index e3d95a68..8514c991 100644
> > > --- a/lib/rendercopy_gen9.c
> > > +++ b/lib/rendercopy_gen9.c
> > > @@ -204,9 +204,15 @@ gen8_bind_buf(struct intel_batchbuffer *batch,
> > > const struct igt_buf *buf,
> > >  	ss->ss0.horizontal_alignment = 1; /* align 4 */
> > >  	if (buf->tiling == I915_TILING_X)
> > >  		ss->ss0.tiled_mode = 2;
> > > -	else if (buf->tiling == I915_TILING_Y)
> > > +	else if (buf->tiling != I915_TILING_NONE)
> > >  		ss->ss0.tiled_mode = 3;
> > >  
> > > +	if (buf->tiling == I915_TILING_Yf)
> > > +		ss->ss5.trmode = 1;
> > > +	else if (buf->tiling == I915_TILING_Ys)
> > > +		ss->ss5.trmode = 2;
> > > +	ss->ss5.mip_tail_start_lod = 1; /* needed with trmode */
> > > +
> > >  	ss->ss8.base_addr = buf->bo->offset64;
> > >  	ss->ss9.base_addr_hi = buf->bo->offset64 >> 32;
> > >  
> > > -- 
> > > 2.17.1
> > >