[igt-dev] [PATCH i-g-t 2/3] lib/igt_fb: Fix stride for render_copy based FB conversions
Imre Deak
imre.deak at intel.com
Fri Apr 30 10:29:12 UTC 2021
On Fri, Apr 30, 2021 at 07:53:02AM +0200, Zbigniew Kempczyński wrote:
> On Thu, Apr 29, 2021 at 10:51:15PM +0300, Ville Syrjälä wrote:
> > On Thu, Apr 29, 2021 at 10:26:27PM +0300, Imre Deak wrote:
> > > When setting up an intel_buf for a render copy operation, the render
> > > buffer stride has to match the framebuffer stride. Not ensuring this may
> > > lead to a mismatch modeset error or an -EBUSY set_tiling IOCTL failure
> > > when trying to change the stride for a framebuffer object.
> >
> > The stride needs to always match, otherwise we're going to be
> > rendering garbage. What used to be the case is that the igt_buf
> > thing was just given the stride and size, and it just set up
> > things so that the render surface covers the entire stride,
> > whether or not the fb is smaller or not.
> >
> > So I guess this is some kind of regression because we're now
> > using the fb->width for something?
>
> Yes, writing intel_buf I was wet behind the ears here. I assumed
> calculating stride according to passed width was enough. As KMS tests
> shows I was wrong and there're cases where stride can be greater than
> aligned width.
Not sure if it can cause an actual problem in the public tree, but yes,
ADL_P needs to pad the stride for CCS FBs where there would be a
mismatch.
> IMO Imre's patch should solve the missing case, thus:
>
> Reviewed-by: Zbigniew Kempczyński <zbigniew.kempczynski at intel.com>
>
> --
> Zbigniew
>
> >
> > >
> > > Cc: Dominik Grzegorzek <dominik.grzegorzek at intel.com>
> > > Cc: Zbigniew Kempczyński <zbigniew.kempczynski at intel.com>
> > > Signed-off-by: Imre Deak <imre.deak at intel.com>
> > > ---
> > > lib/igt_fb.c | 3 ++-
> > > lib/intel_bufops.c | 25 +++++++++++++++++--------
> > > lib/intel_bufops.h | 3 ++-
> > > 3 files changed, 21 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/lib/igt_fb.c b/lib/igt_fb.c
> > > index 5c1670410..209bfacf0 100644
> > > --- a/lib/igt_fb.c
> > > +++ b/lib/igt_fb.c
> > > @@ -2257,7 +2257,8 @@ igt_fb_create_intel_buf(int fd, struct buf_ops *bops,
> > > fb->width, fb->height,
> > > fb->plane_bpp[0], 0,
> > > igt_fb_mod_to_tiling(fb->modifier),
> > > - compression, fb->size);
> > > + compression, fb->size,
> > > + fb->strides[0]);
> > > intel_buf_set_name(buf, name);
> > >
> > > /* Make sure we close handle on destroy path */
> > > diff --git a/lib/intel_bufops.c b/lib/intel_bufops.c
> > > index 3c1cca0cf..5dece5764 100644
> > > --- a/lib/intel_bufops.c
> > > +++ b/lib/intel_bufops.c
> > > @@ -709,7 +709,7 @@ static void __intel_buf_init(struct buf_ops *bops,
> > > struct intel_buf *buf,
> > > int width, int height, int bpp, int alignment,
> > > uint32_t req_tiling, uint32_t compression,
> > > - uint64_t bo_size)
> > > + uint64_t bo_size, int bo_stride)
> > > {
> > > uint32_t tiling = req_tiling;
> > > uint64_t size;
> > > @@ -741,7 +741,9 @@ static void __intel_buf_init(struct buf_ops *bops,
> > > * CCS units, that is 4 * 64 bytes. These 4 CCS units are in
> > > * turn mapped by one L1 AUX page table entry.
> > > */
> > > - if (bops->intel_gen >= 12)
> > > + if (bo_stride)
> > > + buf->surface[0].stride = bo_stride;
> > > + else if (bops->intel_gen >= 12)
> > > buf->surface[0].stride = ALIGN(width * (bpp / 8), 128 * 4);
> > > else
> > > buf->surface[0].stride = ALIGN(width * (bpp / 8), 128);
> > > @@ -765,10 +767,16 @@ static void __intel_buf_init(struct buf_ops *bops,
> > > if (tiling) {
> > > devid = intel_get_drm_devid(bops->fd);
> > > tile_width = get_stride(devid, tiling);
> > > - buf->surface[0].stride = ALIGN(width * (bpp / 8), tile_width);
> > > + if (bo_stride)
> > > + buf->surface[0].stride = bo_stride;
> > > + else
> > > + buf->surface[0].stride = ALIGN(width * (bpp / 8), tile_width);
> > > align_h = tiling == I915_TILING_X ? 8 : 32;
> > > } else {
> > > - buf->surface[0].stride = ALIGN(width * (bpp / 8), alignment ?: 1);
> > > + if (bo_stride)
> > > + buf->surface[0].stride = bo_stride;
> > > + else
> > > + buf->surface[0].stride = ALIGN(width * (bpp / 8), alignment ?: 1);
> > > }
> > >
> > > buf->surface[0].size = buf->surface[0].stride * height;
> > > @@ -816,7 +824,7 @@ void intel_buf_init(struct buf_ops *bops,
> > > uint32_t tiling, uint32_t compression)
> > > {
> > > __intel_buf_init(bops, 0, buf, width, height, bpp, alignment,
> > > - tiling, compression, 0);
> > > + tiling, compression, 0, 0);
> > >
> > > intel_buf_set_ownership(buf, true);
> > > }
> > > @@ -875,7 +883,7 @@ void intel_buf_init_using_handle(struct buf_ops *bops,
> > > uint32_t req_tiling, uint32_t compression)
> > > {
> > > __intel_buf_init(bops, handle, buf, width, height, bpp, alignment,
> > > - req_tiling, compression, 0);
> > > + req_tiling, compression, 0, 0);
> > > }
> > >
> > > /**
> > > @@ -950,7 +958,8 @@ struct intel_buf *intel_buf_create_using_handle_and_size(struct buf_ops *bops,
> > > int bpp, int alignment,
> > > uint32_t req_tiling,
> > > uint32_t compression,
> > > - uint64_t size)
> > > + uint64_t size,
> > > + int stride)
> > > {
> > > struct intel_buf *buf;
> > >
> > > @@ -960,7 +969,7 @@ struct intel_buf *intel_buf_create_using_handle_and_size(struct buf_ops *bops,
> > > igt_assert(buf);
> > >
> > > __intel_buf_init(bops, handle, buf, width, height, bpp, alignment,
> > > - req_tiling, compression, size);
> > > + req_tiling, compression, size, stride);
> > >
> > > return buf;
> > > }
> > > diff --git a/lib/intel_bufops.h b/lib/intel_bufops.h
> > > index 1a3d86925..12c2d5239 100644
> > > --- a/lib/intel_bufops.h
> > > +++ b/lib/intel_bufops.h
> > > @@ -151,7 +151,8 @@ struct intel_buf *intel_buf_create_using_handle_and_size(struct buf_ops *bops,
> > > int bpp, int alignment,
> > > uint32_t req_tiling,
> > > uint32_t compression,
> > > - uint64_t size);
> > > + uint64_t size,
> > > + int stride);
> > > void intel_buf_destroy(struct intel_buf *buf);
> > >
> > > void *intel_buf_cpu_map(struct intel_buf *buf, bool write);
> > > --
> > > 2.27.0
> > >
> > > _______________________________________________
> > > igt-dev mailing list
> > > igt-dev at lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/igt-dev
> >
> > --
> > Ville Syrjälä
> > Intel
> > _______________________________________________
> > igt-dev mailing list
> > igt-dev at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/igt-dev
More information about the igt-dev
mailing list