On 01/10/2014 06:30 PM, Ian Romanick wrote:
> On 01/10/2014 06:24 PM, Kenneth Graunke wrote:
>> On 01/10/2014 05:40 PM, Ian Romanick wrote:
>>> From: Ian Romanick <ian.d.roman...@intel.com>
>>>
>>> Signed-off-by: Ian Romanick <ian.d.roman...@intel.com>
>>> ---
>>> src/mesa/drivers/dri/i965/gen6_clip_state.c | 14 +++++++++-----
>>> 1 file changed, 9 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/src/mesa/drivers/dri/i965/gen6_clip_state.c
>>> b/src/mesa/drivers/dri/i965/gen6_clip_state.c
>>> index 3499e37..ed7afd7 100644
>>> --- a/src/mesa/drivers/dri/i965/gen6_clip_state.c
>>> +++ b/src/mesa/drivers/dri/i965/gen6_clip_state.c
>>> @@ -96,11 +96,15 @@ upload_clip_state(struct brw_context *brw)
>>> dw2 |= (ctx->Transform.ClipPlanesEnabled <<
>>> GEN6_USER_CLIP_CLIP_DISTANCES_SHIFT);
>>>
>>> - if (ctx->ViewportArray[0].X == 0 &&
>>> - ctx->ViewportArray[0].Y == 0 &&
>>> - ctx->ViewportArray[0].Width == (float) fb->Width &&
>>> - ctx->ViewportArray[0].Height == (float) fb->Height) {
>>> - dw2 |= GEN6_CLIP_GB_TEST;
>>> + dw2 |= GEN6_CLIP_GB_TEST;
>>> + for (unsigned i = 0; i < ctx->Const.MaxViewports; i++) {
>>
>> Don't you want to check for actually enabled viewports here? I'm not
>> sure what data the others get, but I imagine it's fairly bogus and thus
>> would result in guardband clipping always getting disabled.
>
> There are three cases, I think...
>
> In the common case, applications call glViewport, and all of the
> viewports get the same data.
Ah, you're right...you do set them all from glViewport...so your code is
fine as-is.
Objection withdrawn :) Thanks for the explanation!
> In another case, applications call glViewportIndexedf (or one if its
> friends), and does rendering using dynamic viewport indexing. In this
> case, we need to check all the viewports (as this code does).
>
> In the final case, the application has called glViewportIndexedf (or one
> if its friends), but renders without using dynamic viewport indexing
> (e.g., no shader contains a static write to gl_ViewportIndex). In this
> case we should be able to only check the first viewport.
>
> However, at this point in the code, we don't have the data to
> differentiate the last two cases. I have a small list of follow-on
> cleaning and optimization... can I add this to that list?
Yeah, I wouldn't worry about that now. We could add it to a "ideas for
optimization someday" list somewhere.
>>> + if (ctx->ViewportArray[i].X != 0 ||
>>> + ctx->ViewportArray[i].Y != 0 ||
>>> + ctx->ViewportArray[i].Width != (float) fb->Width ||
>>> + ctx->ViewportArray[i].Height != (float) fb->Height) {
>>> + dw2 &= ~GEN6_CLIP_GB_TEST;
>>> + break;
>>> + }
>>> }
>>>
>>> /* BRW_NEW_RASTERIZER_DISCARD */
>>>
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
