Re: [igt-dev] [PATCH i-g-t 1/3] lib/i915_blt: Remove src == dst pitch restriction

["Zbigniew Kempczyński" <zbigniew.kempczynski@intel.com>]

On Tue, Dec 13, 2022 at 04:37:26PM +0100, Karolina Stolarek wrote:
> On 12.12.2022 13:50, Zbigniew Kempczyński wrote:
> > During debugging phase we established there's not necessary to enforce
> > same pitch for destination surface in FULL_RESOLVE mode. Relax this
> > condition allowing caller to pass requirement surface configuration.
> > 
> > Signed-off-by: Zbigniew Kempczyński <zbigniew.kempczynski@intel.com>
> > ---
> >   lib/i915/i915_blt.c | 8 +++-----
> >   1 file changed, 3 insertions(+), 5 deletions(-)
> > 
> > diff --git a/lib/i915/i915_blt.c b/lib/i915/i915_blt.c
> > index 3776c56c60..42c28623f9 100644
> > --- a/lib/i915/i915_blt.c
> > +++ b/lib/i915/i915_blt.c
> > @@ -317,13 +317,11 @@ static void fill_data(struct gen12_block_copy_data *data,
> >   	data->dw00.special_mode = __special_mode(blt);
> >   	data->dw00.length = extended_command ? 20 : 10;
> > -	if (__special_mode(blt) == SM_FULL_RESOLVE && blt->src.tiling == T_TILE64) {
> > -		data->dw01.dst_pitch = blt->src.pitch - 1;
> > +	if (__special_mode(blt) == SM_FULL_RESOLVE)
> 
> "blt->src.tiling == T_TILE64" part was introduced in commit cdc0258a4672
> ("lib/i915_blt: Narrow setting same pitch and aux-ccs to Tile64 only").
> I know it's a bit nit-picky, but could we revert that change (as it looks
> like it's not necessary), and have a separate commit that removes
> data->dw01.dst_pitch = blt->src.pitch - 1?

Two reverts would be necessary: cdc0258a467 and 229bb0accbb7 so single commit
against three looks better for me. Tiling inplace testing is still in progress.
If you think better is to revert above and add another commit I'll do it.

> 
> The question remains -- what actually caused the regression for Tile4 and
> xmajor? Was it because of the pitch settings? I run the tests a couple of
> times in the row after applying this patch and everything worked as
> expected.

Yes, dst pitch influences on destination surface state. For cascaded 
src->mid->dst mid.pitch may differ from dst.pitch (tileX -> linear blit).
For inplace there's some side effect which we're debugging now.

--
Zbigniew 

> 
> All the best,
> Karolina
> 
> >   		data->dw01.dst_aux_mode = __aux_mode(&blt->src);
> > -	} else {
> > -		data->dw01.dst_pitch = blt->dst.pitch - 1;
> > +	else
> >   		data->dw01.dst_aux_mode = __aux_mode(&blt->dst);
> > -	}
> > +	data->dw01.dst_pitch = blt->dst.pitch - 1;
> >   	data->dw01.dst_mocs = blt->dst.mocs;
> >   	data->dw01.dst_compression = blt->dst.compression;