[Mesa-dev] [PATCH] llvmpipe: initialize llvmpipe->dirty with LP_NEW_SCISSOR

[Roland Scheidegger]

Am 30.08.2017 um 16:46 schrieb Brian Paul:
> On 08/30/2017 02:43 AM, Roland Scheidegger wrote:
>> Am 30.08.2017 um 04:49 schrieb Brian Paul:
>>> On 08/29/2017 06:17 PM, Roland Scheidegger wrote:
>>>> Looks good to me, thanks.
>>>>
>>>> Reviewed-by: Roland Scheidegger <sroland at vmware.com>
>>>>
>>>> Albeit I'm not quite sure why it is never set here?
>>>
>>> This particular test sets a 0 x 0 scissor region.  All the
>>> pipe_scissor_state members are zero and when the state tracker does a
>>> memcmp() with its copy it sees no difference and skips the call to
>>> pipe->set_scissor_state().
>> Ahh ok. It is pretty much the same as with the null fb not getting
>> initialized then.
>> Albeit technically, I think state trackers should ensure that they
>> always initialize the state, this burden shouldn't be on the drivers.
>> (With the all null fb, this didn't help, because both cso and llvmpipe
>> threw out an initial null fb as no change on their own.)
> 
> That's a good point.  We've never clearly defined how much
> initialization a driver should be responsible for, vs. the state tracker.
> 
> I think we've implicitly been expecting the driver to do everything it
> needs to function, without relying on the state tracker for everything.
>  For example, if we want to render some non-textured triangles and we
> never call the driver's pipe_context::set_sampler_views() or
> bind_sampler_states() functions, we'd expect rendering to work.  And as
> in this case, if set_scissor_state() is never called, I think we'd
> expect rendering to work (though, we are enabling the scissor flag).

Albeit I think this isn't quite the same thing: null sampler views are
"natural" defaults - the driver can't reasonably do anything else, plus,
typically if you render non-textured triangles, these views do not
actually matter no matter what the driver would assume. But if you
enable scissor, then scissor rectangle surely matters. And it's really
not obvious 0-sized scissor is the "natural" default neither (GL's
default of it would be drawable width/height for width and height).
That said, the driver definitely also looks at fault here: its idea of a
default scissor also really was an empty scissor, it just neglected to
calculate the derived values...

So the fix is correct, never setting a scissor in the state tracker is
imho iffy however, iff the scissor rectangle is needed (that is, scissor
test enabled). I think that's just asking for trouble.

Roland


> -Brian
> 
>>
>> Roland
>>
>>
>>>
>>> -Brian
>>>
>>>
>>>> This bug looks similar in nature to the uninitialized fb (see
>>>> 8bfe451ed30918244618608871423b2a72cf9767) and I thought that was
>>>> impossible to hit with gl state tracker...
>>>>
>>>> Roland
>>>>
>>>> Am 28.08.2017 um 22:20 schrieb Brian Paul:
>>>>> If llvmpipe_set_scissor_states() is never called, we still need to be
>>>>> sure
>>>>> that derived scissor/clip state is updated.  As of commit
>>>>> 743ad599a97d09b1
>>>>> that function might not be called.
>>>>>
>>>>> Fixes regressed Piglit gl-1.0-scissor-offscreen -fbo -auto test.
>>>>>
>>>>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=101709
>>>>> ---
>>>>>    src/gallium/drivers/llvmpipe/lp_context.c | 6 ++++++
>>>>>    1 file changed, 6 insertions(+)
>>>>>
>>>>> diff --git a/src/gallium/drivers/llvmpipe/lp_context.c
>>>>> b/src/gallium/drivers/llvmpipe/lp_context.c
>>>>> index 599a9f1..613d60f 100644
>>>>> --- a/src/gallium/drivers/llvmpipe/lp_context.c
>>>>> +++ b/src/gallium/drivers/llvmpipe/lp_context.c
>>>>> @@ -227,6 +227,12 @@ llvmpipe_create_context(struct pipe_screen
>>>>> *screen, void *priv,
>>>>>
>>>>>       lp_reset_counters();
>>>>>
>>>>> +   /* If llvmpipe_set_scissor_states() is never called, we still
>>>>> need to
>>>>> +    * make sure that derived scissor state is computed.
>>>>> +    * See https://bugs.freedesktop.org/show_bug.cgi?id=101709
>>>>> +    */
>>>>> +   llvmpipe->dirty |= LP_NEW_SCISSOR;
>>>>> +
>>>>>       return &llvmpipe->pipe;
>>>>>
>>>>>     fail:
>>>>>
>>>>
>>>
>>
>