[Mesa-dev] [PATCH 6/6] st/mesa: use new cso_set_viewport_dims() helper

[Ilia Mirkin]

On Fri, Feb 12, 2016 at 11:18 AM, Brian Paul <brianp at vmware.com> wrote:
> On 02/12/2016 09:06 AM, Ilia Mirkin wrote:
>>
>> On Fri, Feb 12, 2016 at 10:43 AM, Brian Paul <brianp at vmware.com> wrote:
>>>
>>> ---
>>>   src/mesa/state_tracker/st_cb_bitmap.c     | 16 +++-------------
>>>   src/mesa/state_tracker/st_cb_clear.c      | 13 ++-----------
>>>   src/mesa/state_tracker/st_cb_drawpixels.c | 14 ++------------
>>>   3 files changed, 7 insertions(+), 36 deletions(-)
>>>
>>> diff --git a/src/mesa/state_tracker/st_cb_bitmap.c
>>> b/src/mesa/state_tracker/st_cb_bitmap.c
>>> index 5081639..5a11c0d 100644
>>> --- a/src/mesa/state_tracker/st_cb_bitmap.c
>>> +++ b/src/mesa/state_tracker/st_cb_bitmap.c
>>> @@ -267,19 +267,9 @@ setup_render_state(struct gl_context *ctx,
>>>      }
>>>
>>>      /* viewport state: viewport matching window dims */
>>> -   {
>>> -      const GLboolean invert = st->state.fb_orientation == Y_0_TOP;
>>> -      const GLfloat width = (GLfloat)st->state.framebuffer.width;
>>> -      const GLfloat height = (GLfloat)st->state.framebuffer.height;
>>> -      struct pipe_viewport_state vp;
>>> -      vp.scale[0] =  0.5f * width;
>>> -      vp.scale[1] = height * (invert ? -0.5f : 0.5f);
>>> -      vp.scale[2] = 0.5f;
>>> -      vp.translate[0] = 0.5f * width;
>>> -      vp.translate[1] = 0.5f * height;
>>> -      vp.translate[2] = 0.5f;
>>> -      cso_set_viewport(cso, &vp);
>>> -   }
>>> +   cso_set_viewport_dims(cso, st->state.framebuffer.width,
>>> +                         st->state.framebuffer.height,
>>> +                         st->state.fb_orientation == Y_0_TOP);
>>>
>>>      cso_set_vertex_elements(cso, 3, st->util_velems);
>>>
>>> diff --git a/src/mesa/state_tracker/st_cb_clear.c
>>> b/src/mesa/state_tracker/st_cb_clear.c
>>> index 271f577..27925f9 100644
>>> --- a/src/mesa/state_tracker/st_cb_clear.c
>>> +++ b/src/mesa/state_tracker/st_cb_clear.c
>>> @@ -277,17 +277,8 @@ clear_with_quad(struct gl_context *ctx, unsigned
>>> clear_buffers)
>>>      cso_set_rasterizer(cso, &st->clear.raster);
>>>
>>>      /* viewport state: viewport matching window dims */
>>> -   {
>>> -      const GLboolean invert = (st_fb_orientation(fb) == Y_0_TOP);
>>> -      struct pipe_viewport_state vp;
>>> -      vp.scale[0] = 0.5f * fb_width;
>>> -      vp.scale[1] = fb_height * (invert ? -0.5f : 0.5f);
>>> -      vp.scale[2] = 0.5f;
>>> -      vp.translate[0] = 0.5f * fb_width;
>>> -      vp.translate[1] = 0.5f * fb_height;
>>> -      vp.translate[2] = 0.5f;
>>> -      cso_set_viewport(cso, &vp);
>>> -   }
>>> +   cso_set_viewport_dims(st->cso_context, fb_width, fb_height,
>>> +                         st_fb_orientation(fb) == Y_0_TOP);
>>>
>>>      set_fragment_shader(st);
>>>      cso_set_tessctrl_shader_handle(cso, NULL);
>>> diff --git a/src/mesa/state_tracker/st_cb_drawpixels.c
>>> b/src/mesa/state_tracker/st_cb_drawpixels.c
>>> index 581419e..b1b61c8 100644
>>> --- a/src/mesa/state_tracker/st_cb_drawpixels.c
>>> +++ b/src/mesa/state_tracker/st_cb_drawpixels.c
>>> @@ -596,18 +596,8 @@ draw_textured_quad(struct gl_context *ctx, GLint x,
>>> GLint y, GLfloat z,
>>>      }
>>>
>>>      /* viewport state: viewport matching window dims */
>>> -   {
>>> -      const float w = (float) ctx->DrawBuffer->Width;
>>> -      const float h = (float) ctx->DrawBuffer->Height;
>>> -      struct pipe_viewport_state vp;
>>> -      vp.scale[0] =  0.5f * w;
>>> -      vp.scale[1] = -0.5f * h;
>>> -      vp.scale[2] = 0.5f;
>>> -      vp.translate[0] = 0.5f * w;
>>> -      vp.translate[1] = 0.5f * h;
>>> -      vp.translate[2] = 0.5f;
>>> -      cso_set_viewport(cso, &vp);
>>> -   }
>>> +   cso_set_viewport_dims(cso, ctx->DrawBuffer->Width,
>>> +                         ctx->DrawBuffer->Height, TRUE);
>>
>>
>> Please use _mesa_geometric_width/height here instead of
>> ctx->DrawBuffer (to support the future ARB_framebuffer_no_attachments
>> use-case). [I believe the current patchset for that feature touched
>> this up, but since it's not in, might as well get it right...)
>
>
> Can I do that in a follow-on patch?  I think the two changes should be
> separate.
>
> BTW, if gl_framebuffer::_HasAttachments is false, that means the
> glDrawPixels will effectively be a no-op, right?  So the viewport size won't
> matter.

Doesn't glDrawPixels run the current frag shader? That shader could
have side-effects. It'll also increase GL_SAMPLES_PASSED I think. But
yeah, there's like 0 reason for anyone to use glDrawPixels without
real attachments :) But I think ctx->DrawBuffer will be null, so
crashing isn't so great either.

>
> I see other places where we use gl_framebuffer::Width/Height in the state
> tracker code instead of _mesa_geometric_width/height().  I suspect it may be
> the same story.
>
> Though, I guess it would be more consistent to use
> _mesa_geometric_width/height() everywhere.  What do you think?

You can do it later, or not do it at all and leave it for the
ARB_fb_no_attach enablement patches. I was just coming at it from the
point of view that rebasing the (existing) patches will cause a
conflict anyways (since they will have already updated the first
usage), and it'd be nice to just fix it up here while you're changing
it. But it's fine to leave it for the enablement time too.

Cheers,

  -ilia