Re: [igt-dev] [PATCH i-g-t 1/8] lib/igt_draw: Refactor get_tiling calls

["Dixit, Ashutosh" <ashutosh.dixit@intel.com>]

On Mon, 10 Feb 2020 03:37:54 -0800, Imre Deak wrote:
>
> On Sat, Feb 08, 2020 at 02:36:38PM -0800, Dixit, Ashutosh wrote:
> > On Fri, 07 Feb 2020 11:15:17 -0800, Imre Deak wrote:
> > >  void igt_draw_rect(int fd, drm_intel_bufmgr *bufmgr, drm_intel_context *context,
> > >		   uint32_t buf_handle, uint32_t buf_size, uint32_t buf_stride,
> > > -		   enum igt_draw_method method, int rect_x, int rect_y,
> > > -		   int rect_w, int rect_h, uint32_t color, int bpp)
> > > +		   uint32_t tiling, enum igt_draw_method method,
> > > +		   int rect_x, int rect_y, int rect_w, int rect_h,
> > > +		   uint32_t color, int bpp)
> > >  {
> > > +	uint32_t buf_tiling, swizzle;
> > > +
> > >	struct cmd_data cmd_data = {
> > >		.bufmgr = bufmgr,
> > >		.context = context,
> > > @@ -667,24 +660,32 @@ void igt_draw_rect(int fd, drm_intel_bufmgr *bufmgr, drm_intel_context *context,
> > >		.h = rect_h,
> > >	};
> > >
> > > +	swizzle = I915_BIT_6_SWIZZLE_NONE;
> > > +	if (tiling != I915_TILING_NONE && gem_available_fences(fd)) {
> > > +		gem_get_tiling(fd, buf_handle, &buf_tiling, &swizzle);
> > > +		igt_assert(tiling == buf_tiling);
> > > +	}
> >
> > Probably a nit, but looks a little strange to call gem_get_tiling() to get
> > the swizzle and then doing an assert.
>
> gem_get_tiling() is the way to get the swizzling for a buffer. The
> assert only makes sure that the caller's and kernel's idea of the tiling
> matches. While the callers will pick the tiling (when creating the
> buffer), the swizzling is fixed based on this tiling and platform.

Let me ask a different way, do we even need the new tiling argument to
igt_draw_rect()? Can't we just do:

        if (gem_available_fences(fd))
                gem_get_tiling(fd, buf_handle, &buf_tiling, &swizzle);

Basically if we have the handle we should just be able to query the tiling.

> > Instead, since igt_draw_rect() has only two callers how about moving
> > the gem_get_tiling() call into the callers and passing both tiling and
> > swizzle into igt_draw_rect()?
>
> Why?
>
> >
> > > diff --git a/tests/kms_frontbuffer_tracking.c b/tests/kms_frontbuffer_tracking.c
> > > index 2c765c34..9c2c3a5d 100644
> > > --- a/tests/kms_frontbuffer_tracking.c
> > > +++ b/tests/kms_frontbuffer_tracking.c
> > > @@ -291,6 +291,7 @@ struct {
> > >	int height;
> > >	uint32_t color;
> > >	int bpp;
> > > +	uint32_t tiling;
> >
> > Should not need this if we follow the suggestion above.
>
> This contains the tiling of the buffer as now we won't have a way to
> retrieve the same from the kernel.

Again, won't need these other changes if the tiling arg to igt_draw_rect()
can be eliminated.
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev