[Mesa-dev] [PATCH 1/2] mesa: Set the correct ctx->NewState bitfield for rasterizer discard.

[Kenneth Graunke]

On 12/14/2011 04:10 PM, Marek Olšák wrote:
> On Thu, Dec 15, 2011 at 12:18 AM, Paul Berry <stereotype441 at gmail.com> wrote:
>> On 14 December 2011 15:00, Marek Olšák <maraeo at gmail.com> wrote:
>>>
>>> On Wed, Dec 14, 2011 at 11:25 PM, Paul Berry <stereotype441 at gmail.com>
>>> wrote:
>>>> On 14 December 2011 13:42, Marek Olšák <maraeo at gmail.com> wrote:
>>>>>
>>>>> I think RASTERIZER_DISCARD has nothing to do with transform feedback.
>>>>> I think it's part of the same spec because it's not useful without it.
>>>>> As I understand it, _NEW_TRANSFORM_FEEDBACK is dirty when transform
>>>>> feedback buffer bindings are changed or just enabled/disabled. On the
>>>>> other hand, RASTERIZER_DISCARD enables or disables the rasterizer, so
>>>>> it should fall into the same category as face culling for example. (I
>>>>> even implemented it using face culling on r600)
>>>>>
>>>>> Also there would be no way to know whether _NEW_TRANSFORM_FEEDBACK
>>>>> changes just TFB buffer bindings, or just RASTERIZER_DISCARD, or both.
>>>>>
>>>>> Marek
>>>>
>>>>
>>>> I see where you are coming from--I could have implemented rasterizer
>>>> discard
>>>> on i965 gen6 in the same way.
>>>>
>>>> However, I think there are three compelling reasons to consider
>>>> rasterizer
>>>> discard to be related to transform feedback:
>>>>
>>>> (1) from a user perspective, it really only makes sense to use
>>>> rasterizer
>>>> discard when transform feedback is active.  Thus, it's highly likely
>>>> that
>>>> when the rasterizer discard state is changed, transform feedback
>>>> settings
>>>> will be changed too.
>>>>
>>>> (2) rasterizer discard functionality is described by the same set of
>>>> extensions that enable transform feedback (e.g. EXT_transform_feedback),
>>>> so
>>>> presumably the inventors of these two features thought they were closely
>>>> related.
>>>>
>>>> (3) the enable bit that Mesa uses to keep track of the state of
>>>> rasterizer
>>>> discard is in gl_context::TransformFeedback, not gl_context::Transform.
>>>>
>>>> Item (3) is the most important to me.  One of the scarier things about
>>>> i965
>>>> driver development is that we have to manually keep track of which
>>>> hardware
>>>> configuration commands correspond to which dirty bits.  If we miss a
>>>> dependency, that causes a subtle bug in which a state change might not
>>>> cause
>>>> the hardware state to be updated properly.  These kinds of bugs are not
>>>> well
>>>> exposed by Piglit tests, because most Piglit tests make a large number
>>>> of
>>>> state changes followed by a draw operation, so a missing dependency
>>>> might
>>>> easily go undetected.  The thing that allows me to sleep at night is
>>>> that
>>>> there is a nice one-to-one correspondence between almost all of the
>>>> dirty
>>>> bits and corresponding substructures of gl_context.  For example, the
>>>> _NEW_MODELVIEW dirty bit corresponds to gl_context::ModelView,
>>>> _NEW_PROJECTION corresponds to gl_context::Projection, and so on.  That
>>>> means any time I am worried that I'm not handling dirty bits correctly,
>>>> I
>>>> can visually inspect the code and make sure that the dirty bits I'm
>>>> consulting match up with which elements of gl_context I'm accessing.  If
>>>> we
>>>> leave the code as is, then there's a big undocumented exception to that
>>>> one-to-one correspondence, wherein
>>>> gl_context::TransformFeedback.RasterDiscard is covered by
>>>> _NEW_TRANSFORM,
>>>> not _NEW_TRANSFORM_FEEDBACK, as one would logically guess based on where
>>>> it's located within gl_context.  I'm not confident that I (or other
>>>> developers) will remember this exception once we're no longer in the
>>>> thick
>>>> of implementing transform feedback and rasterizer discard.
>>>>
>>>> It seems to me that we have three possible approaches to choose from
>>>> here:
>>>>
>>>> (a) Go ahead with this patch, and modify r600 code to recompute the face
>>>> culling mode when the _NEW_TRANSFORM_FEEDBACK dirty bit is set.
>>>>
>>>> (b) Don't apply this patch, and instead move RasterDiscard from
>>>> gl_context::TransformFeedback to gl_context::Transform, so that we
>>>> restore
>>>> the one-to-one correspondence between dirty bits and substructures of
>>>> gl_context.
>>>>
>>>> (c) Do nothing, and rely on programmers to remember that RasterDiscard
>>>> is an
>>>> exception to the usual correspondence between dirty bits and
>>>> substructures
>>>> of gl_context.
>>>>
>>>> I'm really not comfortable with (c) because of the risk of future bugs.
>>>> I
>>>> suppose I could be talked into (b) if there's popular support for it,
>>>> but
>>>> it's not my favourite, because as I said earlier, I think there are
>>>> actually
>>>> a lot of good reasons to think of rasterizer discard as related to
>>>> transform
>>>> feedback.  My preference is to do (a).
>>>
>>> (d) Rework the _NEW_* flags such that they roughly match hardware
>>> state groups, not OpenGL state groups. Direct3D 11 and Gallium are two
>>> examples of how it could be done.
>>>
>>> I am for (b) or (d). I would have nothing against (a) if TFB buffer
>>> bindings were not covered by the same flag. It's mainly about the
>>> overhead of state changes, although I admitted there are r600-related
>>> reasons too. Also, Gallium will have rasterizer_discard in the rasterizer
>>> state (once the patches hit master) - that can be changed though.
>>>
>>> Marek
>>
>>
>> I would be happy to review patches to do (d) if someone wants to take that
>> on.  Sadly, I do not have time to work on it myself right now, since I am
>> under deadline pressure to finish OpenGL 3.0 support.
>>
>> As for your concerns about the overhead of state changes caused by putting
>> TFB buffer bindings under the same flag as rasterizer discard, would those
>> concerns be addressed by removing the FLUSH_VERTICES(ctx,
>> _NEW_TRANSFORM_FEEDBACK) call from bind_buffer_range()?  As you pointed out
> 
> Fixing bind_buffer_range wouldn't make the assumed overhead of
> changing RASTERIZER_DISCARD just go away. I'd like RASTERIZER_DISCARD
> to be kept out of the _NEW_TRANSFORM_FEEDBACK flag as long as Gallium
> has it in the rasterizer state. Even _NEW_RASTERIZER_DISCARD would do
> the job. i965 could create transform feedback state from
> _NEW_TRANSFORM_FEEDBACK|_NEW_RASTERIZER_DISCARD, while Gallium could
> use _NEW_TRANSFORM_FEEDBACK for buffer bindings only and
> _NEW_RASTERIZER_DISCARD for the rasterizer state.
> 
> It's the same issue as with _NEW_TEXTURE, which mixes a lot of
> mutually unrelated states.
> 
> Marek

I think _NEW_RASTERIZER_DISCARD is probably the best option for now.
It's simple to do, and being fine grained, is easy to subscribe to in
the right places without really any overhead.