[Mesa-dev] [PATCH] anv/cmd_buffer: check for NULL framebuffer

[Juan A. Suarez Romero]

On Mon, 2019-02-04 at 10:01 -0600, Jason Ekstrand wrote:
> On Mon, Feb 4, 2019 at 3:02 AM Juan A. Suarez Romero <jasuarez at igalia.com> wrote:
> > On Fri, 2019-02-01 at 15:33 -0600, Jason Ekstrand wrote:
> > 
> > > On Fri, Feb 1, 2019 at 10:14 AM Juan A. Suarez Romero <jasuarez at igalia.com> wrote:
> > 
> > > > This can happen when we record a VkCmdDraw in a secondary buffer that
> > 
> > > > was created inheriting from the primary buffer, but with the framebuffer
> > 
> > > > set to NULL in the VkCommandBufferInheritanceInfo.
> > 
> > > > 
> > 
> > > > CC: Jason Ekstrand <jason at jlekstrand.net>
> > 
> > > > ---
> > 
> > > >  src/intel/vulkan/gen7_cmd_buffer.c | 13 +++++++++++--
> > 
> > > >  1 file changed, 11 insertions(+), 2 deletions(-)
> > 
> > > > 
> > 
> > > > diff --git a/src/intel/vulkan/gen7_cmd_buffer.c b/src/intel/vulkan/gen7_cmd_buffer.c
> > 
> > > > index 352892aee33..fe1a47f6ce6 100644
> > 
> > > > --- a/src/intel/vulkan/gen7_cmd_buffer.c
> > 
> > > > +++ b/src/intel/vulkan/gen7_cmd_buffer.c
> > 
> > > > @@ -70,12 +70,21 @@ gen7_cmd_buffer_emit_scissor(struct anv_cmd_buffer *cmd_buffer)
> > 
> > > >        };
> > 
> > > > 
> > 
> > > >        const int max = 0xffff;
> > 
> > > > +
> > 
> > > > +      uint32_t height = 0;
> > 
> > > > +      uint32_t width = 0;
> > 
> > > > +
> > 
> > > > +      if (fb) {
> > 
> > > > +        height = fb->height;
> > 
> > > > +        width = fb->width;
> > 
> > > > +      }
> > 
> > > > +
> > 
> > > >        struct GEN7_SCISSOR_RECT scissor = {
> > 
> > > >           /* Do this math using int64_t so overflow gets clamped correctly. */
> > 
> > > >           .ScissorRectangleYMin = clamp_int64(s->offset.y, 0, max),
> > 
> > > >           .ScissorRectangleXMin = clamp_int64(s->offset.x, 0, max),
> > 
> > > > -         .ScissorRectangleYMax = clamp_int64((uint64_t) s->offset.y + s->extent.height - 1, 0, fb->height - 1),
> > 
> > > > -         .ScissorRectangleXMax = clamp_int64((uint64_t) s->offset.x + s->extent.width - 1, 0, fb->width - 1)
> > 
> > > > +         .ScissorRectangleYMax = clamp_int64((uint64_t) s->offset.y + s->extent.height - 1, 0, height - 1),
> > 
> > > > +         .ScissorRectangleXMax = clamp_int64((uint64_t) s->offset.x + s->extent.width - 1, 0, width - 1)
> > 
> > > 
> > 
> > > If fb == NULL, won't width/height be 0 and this be -1 and we end up clamping to -1?  I think we want to rather make the clamp conditional on having the framebuffer.
> > 
> > > 
> > 
> > 
> > 
> > Right. And as ScissorRectangle(X/Y)Max is an uint value, its value would be
> > 
> > MAX_UINT.
> 
> Which is fine if they specified a scissor that is >= the framebuffer size.  However, if they are actually trying to scissor something, this makes their scissor disappear.  That's bad. :-)
> 
I see :)
So we are setting the clamping to ensure that the rendering happens inside the render area, which must be contained withing the framebuffer area.
This is something that actually the application should ensure, but we do it ourselves because it takes little effort to do it.
But if we do not have a framebuffer, we can't do it, so in this case I think the best compromise is go ahead with the scissor, and let the application to ensure later that the framebuffer is big enough for the rendering area.
I'm sending a V2 patch.
 	J.A.
> --Jason
>  
> > I think as we do not know the framebuffer size (which will be know later, IIUC),
> > 
> > we are emitting the scissor for the full rectangle.
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > > --Jason
> > 
> > >  
> > 
> > > >        };
> > 
> > > > 
> > 
> > > >        if (s->extent.width <= 0 || s->extent.height <= 0) {
> > 
> > 
> > 
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20190208/3e9d9a47/attachment.html>