vrend: use the row-stride when directly reading back to an IOV

Submitted by Gert Wollny on July 18, 2018, 8:25 a.m.

Details

Message ID 20180718082536.27481-1-gert.wollny@collabora.com
State New
Headers show
Series "vrend: use the row-stride when directly reading back to an IOV" ( rev: 1 ) in Virgil 3D

Not browsing as part of any series.

Commit Message

Gert Wollny July 18, 2018, 8:25 a.m.
When guest mesa does a glReadPixel call, it expects the data to be placed
with correct row strides into the readback buffer. While for calls that
invert or require more then one IOV this is done in write_transfer_data,
for a read that only uses a small area and offset - which makes it fit into
the first IOV this was not done. Consequently, in this case configure the
PACK_ROW_LENGTH accordingly.

This fixes a number of piglits:
   arb_copy_image-targets (for texture_2d source or dest, non-compressed)
   arb_get_texture_multisample/sample_position
   arb_texture_rectangle/fbo-blit rect
   arb_get_texture__sub_image-cubemap

Signed-off-by: Gert Wollny <gert.wollny@collabora.com>
---
 src/vrend_renderer.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/vrend_renderer.c b/src/vrend_renderer.c
index 9a22214..ebce41f 100644
--- a/src/vrend_renderer.c
+++ b/src/vrend_renderer.c
@@ -5709,6 +5709,7 @@  static int vrend_transfer_send_readpixels(struct vrend_context *ctx,
    uint32_t h = u_minify(res->base.height0, info->level);
    int elsize = util_format_get_blocksize(res->base.format);
    float depth_scale;
+   int row_stride = info->stride;
 
    vrend_use_program(ctx, 0);
 
@@ -5734,8 +5735,11 @@  static int vrend_transfer_send_readpixels(struct vrend_context *ctx,
    } else {
       send_size = iov[0].iov_len - info->offset;
       data = myptr;
+      if (!row_stride)
+         row_stride = util_format_get_nblocksx(res->base.format, u_minify(res->base.width0, info->level));
    }
 
+
    if (res->readback_fb_id == 0 || (int)res->readback_fb_level != info->level ||
        (int)res->readback_fb_z != info->box->z) {
 
@@ -5771,8 +5775,10 @@  static int vrend_transfer_send_readpixels(struct vrend_context *ctx,
       glPixelStorei(GL_PACK_INVERT_MESA, 1);
    if (!vrend_format_is_ds(res->base.format))
       glReadBuffer(GL_COLOR_ATTACHMENT0_EXT);
-   if (!need_temp && info->stride)
-      glPixelStorei(GL_PACK_ROW_LENGTH, info->stride / elsize);
+
+   if (!need_temp && row_stride) {
+      glPixelStorei(GL_PACK_ROW_LENGTH, row_stride);
+   }
 
    switch (elsize) {
    case 1:
@@ -5845,7 +5851,7 @@  static int vrend_transfer_send_readpixels(struct vrend_context *ctx,
    }
    if (vrend_state.have_mesa_invert && actually_invert)
       glPixelStorei(GL_PACK_INVERT_MESA, 0);
-   if (!need_temp && info->stride)
+   if (!need_temp && row_stride)
       glPixelStorei(GL_PACK_ROW_LENGTH, 0);
    glPixelStorei(GL_PACK_ALIGNMENT, 4);
    if (need_temp) {

Comments

On Wed, Jul 18, 2018 at 10:25:36AM +0200, Gert Wollny wrote:
> When guest mesa does a glReadPixel call, it expects the data to be placed
> with correct row strides into the readback buffer. While for calls that
> invert or require more then one IOV this is done in write_transfer_data,
> for a read that only uses a small area and offset - which makes it fit into
> the first IOV this was not done. Consequently, in this case configure the
> PACK_ROW_LENGTH accordingly.
> 
> This fixes a number of piglits:
>    arb_copy_image-targets (for texture_2d source or dest, non-compressed)
>    arb_get_texture_multisample/sample_position
>    arb_texture_rectangle/fbo-blit rect
>    arb_get_texture__sub_image-cubemap

As you said, this patch seems to fix quite some tests.
I didn't have run all the non-regression tests though but
the code looks good to me.

Also, the code doesn't apply correctly on master.
With the rebase,
Reviewed-by: Elie Tournier <elie.tournier@collabora.com>
> 
> Signed-off-by: Gert Wollny <gert.wollny@collabora.com>
> ---
>  src/vrend_renderer.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/src/vrend_renderer.c b/src/vrend_renderer.c
> index 9a22214..ebce41f 100644
> --- a/src/vrend_renderer.c
> +++ b/src/vrend_renderer.c
> @@ -5709,6 +5709,7 @@ static int vrend_transfer_send_readpixels(struct vrend_context *ctx,
>     uint32_t h = u_minify(res->base.height0, info->level);
>     int elsize = util_format_get_blocksize(res->base.format);
>     float depth_scale;
> +   int row_stride = info->stride;
>  
>     vrend_use_program(ctx, 0);
>  
> @@ -5734,8 +5735,11 @@ static int vrend_transfer_send_readpixels(struct vrend_context *ctx,
>     } else {
>        send_size = iov[0].iov_len - info->offset;
>        data = myptr;
> +      if (!row_stride)
> +         row_stride = util_format_get_nblocksx(res->base.format, u_minify(res->base.width0, info->level));
>     }
>  
> +
>     if (res->readback_fb_id == 0 || (int)res->readback_fb_level != info->level ||
>         (int)res->readback_fb_z != info->box->z) {
>  
> @@ -5771,8 +5775,10 @@ static int vrend_transfer_send_readpixels(struct vrend_context *ctx,
>        glPixelStorei(GL_PACK_INVERT_MESA, 1);
>     if (!vrend_format_is_ds(res->base.format))
>        glReadBuffer(GL_COLOR_ATTACHMENT0_EXT);
> -   if (!need_temp && info->stride)
> -      glPixelStorei(GL_PACK_ROW_LENGTH, info->stride / elsize);
> +
> +   if (!need_temp && row_stride) {
> +      glPixelStorei(GL_PACK_ROW_LENGTH, row_stride);
> +   }
>  
>     switch (elsize) {
>     case 1:
> @@ -5845,7 +5851,7 @@ static int vrend_transfer_send_readpixels(struct vrend_context *ctx,
>     }
>     if (vrend_state.have_mesa_invert && actually_invert)
>        glPixelStorei(GL_PACK_INVERT_MESA, 0);
> -   if (!need_temp && info->stride)
> +   if (!need_temp && row_stride)
>        glPixelStorei(GL_PACK_ROW_LENGTH, 0);
>     glPixelStorei(GL_PACK_ALIGNMENT, 4);
>     if (need_temp) {
> -- 
> 2.17.1
> 
> _______________________________________________
> virglrenderer-devel mailing list
> virglrenderer-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/virglrenderer-devel
Am Dienstag, den 24.07.2018, 16:17 +0100 schrieb Elie Tournier:
> On Wed, Jul 18, 2018 at 10:25:36AM +0200, Gert Wollny wrote:
> > When guest mesa does a glReadPixel call, it expects the data to be
> > placed
> > with correct row strides into the readback buffer. While for calls
> > that
> > invert or require more then one IOV this is done in
> > write_transfer_data,
> > for a read that only uses a small area and offset - which makes it
> > fit into
> > the first IOV this was not done. Consequently, in this case
> > configure the
> > PACK_ROW_LENGTH accordingly.
> > 
> > This fixes a number of piglits:
> >    arb_copy_image-targets (for texture_2d source or dest, non-
> > compressed)
> >    arb_get_texture_multisample/sample_position
> >    arb_texture_rectangle/fbo-blit rect
> >    arb_get_texture__sub_image-cubemap
> 
> As you said, this patch seems to fix quite some tests.
> I didn't have run all the non-regression tests though but
> the code looks good to me.
> 
> Also, the code doesn't apply correctly on master.
> With the rebase,
Which master did you use? for me it applied cleanly to the current
upstream/master  
  edd24783581f6ed3
Author: Po-Hsien Wang <pwang@chromium.org>
Date:   Fri Jul 20 15:01:04 2018 -0700

    Fix create_shader buf boundary check

Thanks for the review 

best, 
Gert

> Reviewed-by: Elie Tournier <elie.tournier@collabora.com>
> > 
> > Signed-off-by: Gert Wollny <gert.wollny@collabora.com>
> > ---
> >  src/vrend_renderer.c | 12 +++++++++---
> >  1 file changed, 9 insertions(+), 3 deletions(-)
> > 
> > diff --git a/src/vrend_renderer.c b/src/vrend_renderer.c
> > index 9a22214..ebce41f 100644
> > --- a/src/vrend_renderer.c
> > +++ b/src/vrend_renderer.c
> > @@ -5709,6 +5709,7 @@ static int
> > vrend_transfer_send_readpixels(struct vrend_context *ctx,
> >     uint32_t h = u_minify(res->base.height0, info->level);
> >     int elsize = util_format_get_blocksize(res->base.format);
> >     float depth_scale;
> > +   int row_stride = info->stride;
> >  
> >     vrend_use_program(ctx, 0);
> >  
> > @@ -5734,8 +5735,11 @@ static int
> > vrend_transfer_send_readpixels(struct vrend_context *ctx,
> >     } else {
> >        send_size = iov[0].iov_len - info->offset;
> >        data = myptr;
> > +      if (!row_stride)
> > +         row_stride = util_format_get_nblocksx(res->base.format,
> > u_minify(res->base.width0, info->level));
> >     }
> >  
> > +
> >     if (res->readback_fb_id == 0 || (int)res->readback_fb_level !=
> > info->level ||
> >         (int)res->readback_fb_z != info->box->z) {
> >  
> > @@ -5771,8 +5775,10 @@ static int
> > vrend_transfer_send_readpixels(struct vrend_context *ctx,
> >        glPixelStorei(GL_PACK_INVERT_MESA, 1);
> >     if (!vrend_format_is_ds(res->base.format))
> >        glReadBuffer(GL_COLOR_ATTACHMENT0_EXT);
> > -   if (!need_temp && info->stride)
> > -      glPixelStorei(GL_PACK_ROW_LENGTH, info->stride / elsize);
> > +
> > +   if (!need_temp && row_stride) {
> > +      glPixelStorei(GL_PACK_ROW_LENGTH, row_stride);
> > +   }
> >  
> >     switch (elsize) {
> >     case 1:
> > @@ -5845,7 +5851,7 @@ static int
> > vrend_transfer_send_readpixels(struct vrend_context *ctx,
> >     }
> >     if (vrend_state.have_mesa_invert && actually_invert)
> >        glPixelStorei(GL_PACK_INVERT_MESA, 0);
> > -   if (!need_temp && info->stride)
> > +   if (!need_temp && row_stride)
> >        glPixelStorei(GL_PACK_ROW_LENGTH, 0);
> >     glPixelStorei(GL_PACK_ALIGNMENT, 4);
> >     if (need_temp) {
> > -- 
> > 2.17.1
> > 
> > _______________________________________________
> > virglrenderer-devel mailing list
> > virglrenderer-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/virglrenderer-devel
On Tue, Jul 24, 2018 at 07:43:26PM +0200, Gert Wollny wrote:
> Am Dienstag, den 24.07.2018, 16:17 +0100 schrieb Elie Tournier:
> > On Wed, Jul 18, 2018 at 10:25:36AM +0200, Gert Wollny wrote:
> > > When guest mesa does a glReadPixel call, it expects the data to be
> > > placed
> > > with correct row strides into the readback buffer. While for calls
> > > that
> > > invert or require more then one IOV this is done in
> > > write_transfer_data,
> > > for a read that only uses a small area and offset - which makes it
> > > fit into
> > > the first IOV this was not done. Consequently, in this case
> > > configure the
> > > PACK_ROW_LENGTH accordingly.
> > > 
> > > This fixes a number of piglits:
> > >    arb_copy_image-targets (for texture_2d source or dest, non-
> > > compressed)
> > >    arb_get_texture_multisample/sample_position
> > >    arb_texture_rectangle/fbo-blit rect
> > >    arb_get_texture__sub_image-cubemap
> > 
> > As you said, this patch seems to fix quite some tests.
> > I didn't have run all the non-regression tests though but
> > the code looks good to me.
> > 
> > Also, the code doesn't apply correctly on master.
> > With the rebase,
> Which master did you use? for me it applied cleanly to the current
> upstream/master  
>   edd24783581f6ed3
> Author: Po-Hsien Wang <pwang@chromium.org>
> Date:   Fri Jul 20 15:01:04 2018 -0700
> 
>     Fix create_shader buf boundary check
> 
> Thanks for the review 
> 
> best, 
> Gert

Oups I reviewed the v2 not this one.

I tried with the current master (34809ef). Still no luck.
But the rebase should be simple.
> 
> > Reviewed-by: Elie Tournier <elie.tournier@collabora.com>
> > > 
> > > Signed-off-by: Gert Wollny <gert.wollny@collabora.com>
> > > ---
> > >  src/vrend_renderer.c | 12 +++++++++---
> > >  1 file changed, 9 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/src/vrend_renderer.c b/src/vrend_renderer.c
> > > index 9a22214..ebce41f 100644
> > > --- a/src/vrend_renderer.c
> > > +++ b/src/vrend_renderer.c
> > > @@ -5709,6 +5709,7 @@ static int
> > > vrend_transfer_send_readpixels(struct vrend_context *ctx,
> > >     uint32_t h = u_minify(res->base.height0, info->level);
> > >     int elsize = util_format_get_blocksize(res->base.format);
> > >     float depth_scale;
> > > +   int row_stride = info->stride;
> > >  
> > >     vrend_use_program(ctx, 0);
> > >  
> > > @@ -5734,8 +5735,11 @@ static int
> > > vrend_transfer_send_readpixels(struct vrend_context *ctx,
> > >     } else {
> > >        send_size = iov[0].iov_len - info->offset;
> > >        data = myptr;
> > > +      if (!row_stride)
> > > +         row_stride = util_format_get_nblocksx(res->base.format,
> > > u_minify(res->base.width0, info->level));
> > >     }
> > >  
> > > +
> > >     if (res->readback_fb_id == 0 || (int)res->readback_fb_level !=
> > > info->level ||
> > >         (int)res->readback_fb_z != info->box->z) {
> > >  
> > > @@ -5771,8 +5775,10 @@ static int
> > > vrend_transfer_send_readpixels(struct vrend_context *ctx,
> > >        glPixelStorei(GL_PACK_INVERT_MESA, 1);
> > >     if (!vrend_format_is_ds(res->base.format))
> > >        glReadBuffer(GL_COLOR_ATTACHMENT0_EXT);
> > > -   if (!need_temp && info->stride)
> > > -      glPixelStorei(GL_PACK_ROW_LENGTH, info->stride / elsize);
> > > +
> > > +   if (!need_temp && row_stride) {
> > > +      glPixelStorei(GL_PACK_ROW_LENGTH, row_stride);
> > > +   }
> > >  
> > >     switch (elsize) {
> > >     case 1:
> > > @@ -5845,7 +5851,7 @@ static int
> > > vrend_transfer_send_readpixels(struct vrend_context *ctx,
> > >     }
> > >     if (vrend_state.have_mesa_invert && actually_invert)
> > >        glPixelStorei(GL_PACK_INVERT_MESA, 0);
> > > -   if (!need_temp && info->stride)
> > > +   if (!need_temp && row_stride)
> > >        glPixelStorei(GL_PACK_ROW_LENGTH, 0);
> > >     glPixelStorei(GL_PACK_ALIGNMENT, 4);
> > >     if (need_temp) {
> > > -- 
> > > 2.17.1
> > > 
> > > _______________________________________________
> > > virglrenderer-devel mailing list
> > > virglrenderer-devel@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/virglrenderer-devel
Am Mittwoch, den 25.07.2018, 11:16 +0100 schrieb Elie Tournier:
> On Tue, Jul 24, 2018 at 07:43:26PM +0200, Gert Wollny wrote:
> > Am Dienstag, den 24.07.2018, 16:17 +0100 schrieb Elie Tournier:
> > > On Wed, Jul 18, 2018 at 10:25:36AM +0200, Gert Wollny wrote:
> > > > When guest mesa does a glReadPixel call, it expects the data to
> > > > be
> > > > placed
> > > > with correct row strides into the readback buffer. While for
> > > > calls
> > > > that
> > > > invert or require more then one IOV this is done in
> > > > write_transfer_data,
> > > > for a read that only uses a small area and offset - which makes
> > > > it
> > > > fit into
> > > > the first IOV this was not done. Consequently, in this case
> > > > configure the
> > > > PACK_ROW_LENGTH accordingly.
> > > > 
> > > > This fixes a number of piglits:
> > > >    arb_copy_image-targets (for texture_2d source or dest, non-
> > > > compressed)
> > > >    arb_get_texture_multisample/sample_position
> > > >    arb_texture_rectangle/fbo-blit rect
> > > >    arb_get_texture__sub_image-cubemap
> > > 
> > > As you said, this patch seems to fix quite some tests.
> > > I didn't have run all the non-regression tests though but
> > > the code looks good to me.
> > > 
> > > Also, the code doesn't apply correctly on master.
> > > With the rebase,
> > 
> > Which master did you use? for me it applied cleanly to the current
> > upstream/master  
> >   edd24783581f6ed3
> > Author: Po-Hsien Wang <pwang@chromium.org>
> > Date:   Fri Jul 20 15:01:04 2018 -0700
> > 
> >     Fix create_shader buf boundary check
> > 
> > Thanks for the review 
> > 
> > best, 
> > Gert
> 
> Oups I reviewed the v2 not this one.
The v2 has just one empty line added less. 
> 
> I tried with the current master (34809ef). Still no luck.
> But the rebase should be simple.

Anyway, I checked, and yes, "git am" doesn't like it because of ~200
lines fuzz, patch works fine, a branch with this applied is at

https://gitlab.collabora.com/virgl-es/virglrenderer/tree/gerddie-fix-st
ride

Cheers, 
Gert


> > 
> > > Reviewed-by: Elie Tournier <elie.tournier@collabora.com>
> > > > 
> > > > Signed-off-by: Gert Wollny <gert.wollny@collabora.com>
> > > > ---
> > > >  src/vrend_renderer.c | 12 +++++++++---
> > > >  1 file changed, 9 insertions(+), 3 deletions(-)
> > > > 
> > > > diff --git a/src/vrend_renderer.c b/src/vrend_renderer.c
> > > > index 9a22214..ebce41f 100644
> > > > --- a/src/vrend_renderer.c
> > > > +++ b/src/vrend_renderer.c
> > > > @@ -5709,6 +5709,7 @@ static int
> > > > vrend_transfer_send_readpixels(struct vrend_context *ctx,
> > > >     uint32_t h = u_minify(res->base.height0, info->level);
> > > >     int elsize = util_format_get_blocksize(res->base.format);
> > > >     float depth_scale;
> > > > +   int row_stride = info->stride;
> > > >  
> > > >     vrend_use_program(ctx, 0);
> > > >  
> > > > @@ -5734,8 +5735,11 @@ static int
> > > > vrend_transfer_send_readpixels(struct vrend_context *ctx,
> > > >     } else {
> > > >        send_size = iov[0].iov_len - info->offset;
> > > >        data = myptr;
> > > > +      if (!row_stride)
> > > > +         row_stride = util_format_get_nblocksx(res-
> > > > >base.format,
> > > > u_minify(res->base.width0, info->level));
> > > >     }
> > > >  
> > > > +
> > > >     if (res->readback_fb_id == 0 || (int)res->readback_fb_level 
> > > > !=
> > > > info->level ||
> > > >         (int)res->readback_fb_z != info->box->z) {
> > > >  
> > > > @@ -5771,8 +5775,10 @@ static int
> > > > vrend_transfer_send_readpixels(struct vrend_context *ctx,
> > > >        glPixelStorei(GL_PACK_INVERT_MESA, 1);
> > > >     if (!vrend_format_is_ds(res->base.format))
> > > >        glReadBuffer(GL_COLOR_ATTACHMENT0_EXT);
> > > > -   if (!need_temp && info->stride)
> > > > -      glPixelStorei(GL_PACK_ROW_LENGTH, info->stride /
> > > > elsize);
> > > > +
> > > > +   if (!need_temp && row_stride) {
> > > > +      glPixelStorei(GL_PACK_ROW_LENGTH, row_stride);
> > > > +   }
> > > >  
> > > >     switch (elsize) {
> > > >     case 1:
> > > > @@ -5845,7 +5851,7 @@ static int
> > > > vrend_transfer_send_readpixels(struct vrend_context *ctx,
> > > >     }
> > > >     if (vrend_state.have_mesa_invert && actually_invert)
> > > >        glPixelStorei(GL_PACK_INVERT_MESA, 0);
> > > > -   if (!need_temp && info->stride)
> > > > +   if (!need_temp && row_stride)
> > > >        glPixelStorei(GL_PACK_ROW_LENGTH, 0);
> > > >     glPixelStorei(GL_PACK_ALIGNMENT, 4);
> > > >     if (need_temp) {
> > > > -- 
> > > > 2.17.1
> > > > 
> > > > _______________________________________________
> > > > virglrenderer-devel mailing list
> > > > virglrenderer-devel@lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/virglrenderer-de
> > > > vel