[Mesa-dev] [PATCH] mesa: Change driver interface for ARB_viewport_array

[Marek Olšák]

Hi Ian,

Gallium uses the following interface:

   void (*set_scissor_states)( struct pipe_context *,
                               unsigned start_slot,
                               unsigned num_scissors,
                               const struct pipe_scissor_state * );

   void (*set_viewport_states)( struct pipe_context *,
                                unsigned start_slot,
                                unsigned num_viewports,
                                const struct pipe_viewport_state *);

There is always a fixed number of viewports and scissors (it depends
on the driver, usually 16). The interface allows changing any subset
of consecutive viewports/scissors (the range being changed is
[start_slot, start_slot+num-1]). E.g. if I want to update viewports 14
and 15, I can do so by calling set_viewport_states(ctx, 14, 2,
viewports).

"GLbitfield  _DirtyViewports;" would certainly be very useful.

Marek

On Mon, Nov 4, 2013 at 7:43 PM, Ian Romanick <idr at freedesktop.org> wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> On 11/01/2013 04:12 PM, Francisco Jerez wrote:
>> Ian Romanick <idr at freedesktop.org> writes:
>>
>>> On 11/01/2013 02:04 PM, Courtney Goeltzenleuchter wrote:
>>>> [...]
>>> More often, the dd_function_table functions allow core Mesa to
>>> signal the driver driver that something happened... and the
>>> driver may need to do something in response.  For DRI2 drivers,
>>> the Viewport function is a good example.  DRI2 drivers use that
>>> signal as a hint that the window may have been resized.
>>>
>>> Other dd_function_table functions are used by core Mesa to
>>> request the driver create or destroy some resource (e.g., texture
>>> storage).
>>>
>>> If it weren't for the way nouveau used the DepthRange and Scissor
>>> hooks, I think we could just about get rid of them altogether.  I
>>> wish that driver just used the dirty state bits like everyone
>>> else. :(
>>
>> Cases like the new dd_function_table::Scissor and ::Viewport hooks
>> introduced in this patch series are the reason why nouveau tends
>> to prefer overriding the dd_function_table to keep track of state
>> changes rather than looking at the ctx->NewState bits, because the
>> latter tend to be very coarse-grained -- e.g. there's one big
>> _NEW_TEXTURE flag for all the state of all texture units while
>> nouveau is able to update a subset of the texture state
>> independently for only those texture units that have changed.
>>
>> With the dd_function_table interface proposed in this patch series
>> it would be possible for the drivers to update the state of each
>> viewport in the viewport array independently, which might be
>> beneficial for some hardware someday, removing ::DepthRange and
>> ::Scissor would preclude that possibility.
>
> Right... I wonder if we might be better of just tracking a bit per
> viewport in the gl_context.  I'm assuming we'll end up with something
> like:
>
>     struct gl_viewport_attrib Viewports[MAX_VIEWPORTS];
>
> in the gl_context.  It would be easy to add
>
>     GLbitfield  _DirtyViewports;
>
> along side it.  The various Viewport and DepthRange functions would
> set bits in that field along with _NEW_VIEWPORT.  Drivers that care
> could examine (and clear) those bits.
>
> We'd do similar for Scissor.
>
> Looking at the i965 hardware (and our driver architecture), I believe
> we have to upload all of the viewports anytime there's a change
> anyway.  The viewports are just stored as an array in a BO.  See
> gen7_upload_sf_clip_viewport and similar functions.
>
> Marek:  Do you know what Radeon / Gallium want?
>
>> That said, I don't think nouveau is ever going to care in this
>> particular case, I don't mind if you want to get rid of them.  The
>> following patches:
>>
>> [PATCH 1/2] nouveau: Use _NEW_VIEWPORT instead of hooking through
>> dd_function_table [PATCH 2/2] nouveau: Use _NEW_SCISSOR instead of
>> hooking through dd_function_table
>>
>> are Reviewed-by: Francisco Jerez <currojerez at riseup.net>.
>
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.13 (GNU/Linux)
>
> iEYEARECAAYFAlJ36tgACgkQX1gOwKyEAw+M6wCgkdljRbs5qESfZMmz4TycebY/
> ypoAn0Hw3OtNgYcMfR1dBD5qomrsFHO9
> =wRW3
> -----END PGP SIGNATURE-----