[igt-dev] [PATCH i-g-t 2/3] lib/igt_fb: Fix stride for render_copy based FB conversions
Zbigniew Kempczyński
zbigniew.kempczynski at intel.com
Fri Apr 30 05:53:02 UTC 2021
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.
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