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

Dixit, Ashutosh
2020-02-12T04:29:12Z

On Tue, 11 Feb 2020 11:18:45 -0800, Imre Deak wrote:
> On Tue, Feb 11, 2020 at 11:10:13AM -0800, Dixit, Ashutosh wrote:
> > 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.
>
> We do need to handle different tilings even if
> !gem_available_fences(fd), so we need the tiling param. Note that HW
> detiling support via GGTT != tiling support by display,gt engines.

OK:

Reviewed-by: Ashutosh Dixit <ashutosh.dixit@intel.com>