virgl: Set bind when creating temp resource.

Submitted by Lepton Wu on March 18, 2019, 9:44 p.m.

Details

Message ID 20190318214424.26276-1-lepton@chromium.org
State New
Headers show
Series "virgl: Set bind when creating temp resource." ( rev: 1 ) in Mesa

Not browsing as part of any series.

Commit Message

Lepton Wu March 18, 2019, 9:44 p.m.
virgl render complains about "Illegal resource" when running
dEQP-EGL.functional.color_clears.single_context.gles2.rgb888_window,
the reason is that a zero bind value was given for temp resource.

Signed-off-by: Lepton Wu <lepton@chromium.org>
---
 src/gallium/drivers/virgl/virgl_texture.c | 1 +
 1 file changed, 1 insertion(+)

Patch hide | download patch | download mbox

diff --git a/src/gallium/drivers/virgl/virgl_texture.c b/src/gallium/drivers/virgl/virgl_texture.c
index 231319899e0..563dbacba7e 100644
--- a/src/gallium/drivers/virgl/virgl_texture.c
+++ b/src/gallium/drivers/virgl/virgl_texture.c
@@ -66,6 +66,7 @@  static void virgl_init_temp_resource_from_box(struct pipe_resource *res,
                                               unsigned level, unsigned flags)
 {
    memset(res, 0, sizeof(*res));
+   res->bind = orig->bind;
    res->format = orig->format;
    res->width0 = box->width;
    res->height0 = box->height;

Comments

On Mon, 2019-03-18 at 14:44 -0700, Lepton Wu wrote:
> virgl render complains about "Illegal resource" when running
> dEQP-EGL.functional.color_clears.single_context.gles2.rgb888_window,
> the reason is that a zero bind value was given for temp resource.
> 
> Signed-off-by: Lepton Wu <lepton@chromium.org>
> ---
>  src/gallium/drivers/virgl/virgl_texture.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/src/gallium/drivers/virgl/virgl_texture.c
> b/src/gallium/drivers/virgl/virgl_texture.c
> index 231319899e0..563dbacba7e 100644
> --- a/src/gallium/drivers/virgl/virgl_texture.c
> +++ b/src/gallium/drivers/virgl/virgl_texture.c
> @@ -66,6 +66,7 @@ static void
> virgl_init_temp_resource_from_box(struct pipe_resource *res,
>                                                unsigned level,
> unsigned flags)
>  {
>     memset(res, 0, sizeof(*res));
> +   res->bind = orig->bind;
>     res->format = orig->format;
>     res->width0 = box->width;
>     res->height0 = box->height;

I have a similar-ish patch for the same issue in a branch I'll be
sending out soon:

https://gitlab.freedesktop.org/kusma/mesa/commit/6c19b6b98025a1be31eabdb559709b18eecdbafa#note_132855

Now, as Dave pointed out, there might be some more cases missing in my
patch. I also tried your approach, and it works for me. But I'm not
entirely sure it's the right one; for instance I don't think we'd ever
want to carry flags like PIPE_BIND_DISPLAY_TARGET and
PIPE_BIND_BLENDABLE forward.

Perhaps the right thing is to do something like:

res->bind = orig->bind & (PIPE_BIND_DEPTH_STENCIL |
                          PIPE_BIND_RENDER_TARGET);

But I'm not sure if that's enough; what if we get a surface with
PIPE_BIND_SHADER_IMAGE set? We probably still want to use
PIPE_BIND_RENDER_TARGET for the blit then...

On Mon, 2019-04-01 at 12:43 -0700, Lepton Wu wrote:
> 
> 
> On Tue, Mar 19, 2019 at 4:29 AM Erik Faye-Lund <
> erik.faye-lund@collabora.com> wrote:
> > On Mon, 2019-03-18 at 14:44 -0700, Lepton Wu wrote:
> > > virgl render complains about "Illegal resource" when running
> > > dEQP-
> > EGL.functional.color_clears.single_context.gles2.rgb888_window,
> > > the reason is that a zero bind value was given for temp resource.
> > > 
> > > Signed-off-by: Lepton Wu <lepton@chromium.org>
> > > ---
> > >  src/gallium/drivers/virgl/virgl_texture.c | 1 +
> > >  1 file changed, 1 insertion(+)
> > > 
> > > diff --git a/src/gallium/drivers/virgl/virgl_texture.c
> > > b/src/gallium/drivers/virgl/virgl_texture.c
> > > index 231319899e0..563dbacba7e 100644
> > > --- a/src/gallium/drivers/virgl/virgl_texture.c
> > > +++ b/src/gallium/drivers/virgl/virgl_texture.c
> > > @@ -66,6 +66,7 @@ static void
> > > virgl_init_temp_resource_from_box(struct pipe_resource *res,
> > >                                                unsigned level,
> > > unsigned flags)
> > >  {
> > >     memset(res, 0, sizeof(*res));
> > > +   res->bind = orig->bind;
> > >     res->format = orig->format;
> > >     res->width0 = box->width;
> > >     res->height0 = box->height;
> > 
> > I have a similar-ish patch for the same issue in a branch I'll be
> > sending out soon:
> > 
> > https://gitlab.freedesktop.org/kusma/mesa/commit/6c19b6b98025a1be31eabdb559709b18eecdbafa#note_132855
> > 
> > Now, as Dave pointed out, there might be some more cases missing in
> > my
> > patch. I also tried your approach, and it works for me. But I'm not
> > entirely sure it's the right one; for instance I don't think we'd
> > ever
> > want to carry flags like PIPE_BIND_DISPLAY_TARGET and
> > PIPE_BIND_BLENDABLE forward.
> > 
> > Perhaps the right thing is to do something like:
> > 
> > res->bind = orig->bind & (PIPE_BIND_DEPTH_STENCIL |
> >                           PIPE_BIND_RENDER_TARGET);
> > 
> > But I'm not sure if that's enough; what if we get a surface with
> > PIPE_BIND_SHADER_IMAGE set? We probably still want to use
> > PIPE_BIND_RENDER_TARGET for the blit then...
> 
> How about this, let's deal with  PIPE_BIND_DEPTH_STENCIL
> and PIPE_BIND_RENDER_TARGET first in this patch, also add a warning
> here 
> if we see any other bind so later people  can fix things based on
> real cases. Then at least we fix something in this patch.

Sounds good to me.