[Mesa-dev] [PATCH 1/8] gallium: Add a cap for offset_units_unscaled

[Axel Davy]

On 15/06/2016 03:04, Roland Scheidegger wrote:
> Am 15.06.2016 um 01:08 schrieb Axel Davy:
>> On 15/06/2016 00:21, Roland Scheidegger wrote:
>>> Am 14.06.2016 um 23:33 schrieb Axel Davy:
>>>> diff --git a/src/gallium/include/pipe/p_state.h
>>>> b/src/gallium/include/pipe/p_state.h
>>>> index 396f563..7dce80a 100644
>>>> --- a/src/gallium/include/pipe/p_state.h
>>>> +++ b/src/gallium/include/pipe/p_state.h
>>>> @@ -139,6 +139,13 @@ struct pipe_rasterizer_state
>>>>       unsigned clip_halfz:1;
>>>>         /**
>>>> +    * When true do not scale offset_units and use same rules for
>>>> unorm and
>>>> +    * float depth buffers (D3D9). When false use GL/D3D1X behaviour.
>>>> +    * This depends on PIPE_CAP_POLYGON_OFFSET_UNITS_UNSCALED.
>>>> +    */
>>>> +   unsigned offset_units_unscaled;
>>>> +
>>>> +   /**
>>>>        * Enable bits for clipping half-spaces.
>>>>        * This applies to both user clip planes and shader clip distances.
>>>>        * Note that if the bound shader exports any clip distances, these
>>>>
>>> I don't like this. Generally, for unorm formats, you can easily enough
>>> translate this from d3d9 to gl (or d3d10) rules (but yes, obviously it's
>>> going to be format dependent). (With one big caveat, in general not all
>>> gl drivers think the minimum resolvable difference is the same, that
>>> might range from 2^-22 to 2^-24 for 24bit unorm depth for instance, and
>>> I don't think it's quite consistent with gallium drivers neither).
>>>
>>> You are right though for float depth the formula is different, and you
>>> can't translate it. But do you really need float depth buffer support?
>>> AFAIK no d3d9 app really depends on it, everything can fall back to d24.
>>>
>>> Roland
>>>
>> Hi,
>>
>>
>> That's true float depth buffer do not seem to be widely used in d3d9.
>>
>> The two float depth buffers available in d3d9, as far as I know, are
>> D32F_LOCKABLE and D24FS8.
>>
>> We can see the support for those and other depth buffers here (note that
>> these are mainly old cards):
>>
>> http://zp.amsnet.pl/cdragan/query.php?dxversion=9&feature=formats&featuregroup=selected&adaptergroup=all&featureselected[]=45&featureselected[]=44&featureselected[]=41&featureselected[]=42&featureselected[]=43&featureselected[]=40&featureselected[]=39&featureselected[]=46&resource=SURFACE&usage=DEPTHSTENCIL&orientation=horizontal
>>
>>
>> It is likely not a requirement for any game to support these formats.
>>
>>
>> We could ignore these formats, and add to gallium a way to get the
>> minimum resolvable difference per depth buffer format from drivers. We
>> considered this option.
>>
>>
>> That said, the driver is the best location to know about the minimum
>> resolvable difference, and we made the choice to let the driver do the
>> scaling instead of doing it based on some driver query in the state
>> tracker.
>>
>> As for floating point depth buffers behaviour, I understand for some
>> drivers it may be harder than for others to implement.
>>
>> That doesn't seem however a reason to drop floating depth buffer support
>> in Gallium Nine. D32F_LOCKABLE is particularly useful for debugging,
>> being lockable, it can be used to show depth buffer content after some
>> draw calls for d3d on windows, and compare with nine. And some apps may
>> use it for some particular effects.
>>
>> I'd be ok if we make the float depth buffer part of
>> offset_units_unscaled optional given how rare the combination float
>> depth buffers + depth bias must be used. However if hw can do it, I see
>> no reason why we wouldn't support the capability?
> On second look, it doesn't really look too bad (and fwiw we actually
> could probably put it to use here if we'd support it in llvmpipe).
> Albeit,
> unsigned offset_units_unscaled;
> needs to be
> unsigned offset_units_unscaled:1;
Good catch, this was the reason I had put it in this place of the structure,
but somehow forgot the :1 ...
>
> I'm just very sceptical when it comes to capabilities solely to the
> benefit of fringe state trackers (and everything not st/mesa counts
> here). It usually means driver authors aren't going to bother. And you
> probably can't implement it in all drivers yourselves even if the hw
> could do it.
>
> That said, I'm ok with this if there's no objections from others.
>
> Roland
>