[Mesa-dev] [PATCH] tgsi: lowering support for alpha test

[Rob Clark]

On Fri, Dec 19, 2014 at 5:06 PM, Brian Paul <brianp at vmware.com> wrote:
> On 12/19/2014 02:47 PM, Rob Clark wrote:
>>
>> On Fri, Dec 19, 2014 at 4:26 PM, Ilia Mirkin <imirkin at alum.mit.edu> wrote:
>>>
>>> On Fri, Dec 19, 2014 at 4:04 PM, Rob Clark <robdclark at gmail.com> wrote:
>>>>
>>>> On Fri, Dec 19, 2014 at 3:50 PM, Ilia Mirkin <imirkin at alum.mit.edu>
>>>> wrote:
>>>>>
>>>>> On Fri, Dec 19, 2014 at 2:11 PM, Rob Clark <robdclark at gmail.com> wrote:
>>>>>>
>>>>>> From: Rob Clark <robclark at freedesktop.org>
>>>>>>
>>>>>> This emulates alpha-test with a compare + KILL_IF.  The alpha-ref
>>>>>> value
>>>>>> is passed to the shader via constant tagged with new ALPHAREF
>>>>>> semantic.
>>>>>> For example:
>>>>>>
>>>>>>    FRAG
>>>>>>    PROPERTY FS_COLOR0_WRITES_ALL_CBUFS 1
>>>>>>    DCL IN[0], COLOR, COLOR
>>>>>>    DCL OUT[0], COLOR
>>>>>>      0: MOV OUT[0], IN[0]
>>>>>>      1: END
>>>>>>
>>>>>> with alpha-func PIPE_FUNC_LESS, becomes:
>>>>>>
>>>>>>    FRAG
>>>>>>    PROPERTY FS_COLOR0_WRITES_ALL_CBUFS 1
>>>>>>    DCL IN[0], COLOR, COLOR
>>>>>>    DCL OUT[0], COLOR
>>>>>>    IMM[0] FLT32 {    0.0000,     1.0000,   128.0000,     0.0000}
>>>>>>    DCL TEMP[0]
>>>>>>    DCL TEMP[1]
>>>>>>    DCL CONST[0], ALPHAREF
>>>>>
>>>>>
>>>>> IMO this is pretty confusing. A SV makes more sense, no? Otherwise
>>>>> you'd have a situation where
>>>>>
>>>>> DCL CONST[0]
>>>>> DCL CONST[1]
>>>>> DCL CONST[2], ALPHAREF
>>>>>
>>>>> MOV TEMP[0], CONST[ADDR[0].x]
>>>>>
>>>>> Could potentially be expected to move the alpharef into the temp,
>>>>> which is... odd. Also I don't think there's any precendent for tagging
>>>>> const's with semantics.
>>>>
>>>>
>>>> it is a bit odd, but isn't an out of bounds access undefined anyways?
>>>
>>>
>>> With ARB_robust_buffer_access, it's defined as either 0 or some value
>>> in the constbuf... or some wishy-washy thing like that.
>>
>>
>> well, that won't work without bounds checking in the shader.. but
>> unless there are as many piglit tests as there are for
>> bordercolor+GL_WRAP I'd be inclined just to say: be happy you can play
>> your gl game on gles hw :-P
>>
>>>>
>>>> And yes, I don't think there is any precedent for tagging const's w/
>>>> semantic, it was a rather logical choice..  ie. if you are lowering
>>>> alpha-test, that means you don't have some special register/etc to
>>>> store alpharef, therefore driver is going to treat it as a normal
>>>> const.  It just needs some way to know where to stuff the const val.
>>>> Treating it as something other than a const didn't seem to make much
>>>> sense from the perspective of the consumer of the resulting tgsi.
>>>
>>>
>>> But it does. What if CONST[0] is a UBO (although in that case it may
>>> actually end up as CONST[0][0] or something) -- are you going to
>>> modify the buffer data there? These are "system" values, not user
>>> values. I think they make more sense as SV's... for example all the
>>> sample position things are stored in a driver constbuf on nouveau, but
>>> are exposed as SV's -- it doesn't always have to be a register.
>>>
>>
>> why would a CONST inserted by tgsi lower pass be in a UBO
>>
>>>>
>>>>> The other thing to consider is whether each of these things should be
>>>>> semantics, or whether there should be a
>>>>>
>>>>> TGSI_SEMANTIC_DRIVER
>>>>>
>>>>> Which is then indexed in a custom way (in this case, however
>>>>> tgsi_lowering decides). Many drivers end up with driver-internal
>>>>> constbufs full of convenient values, this would be a way to represent
>>>>> that.
>>>>
>>>>
>>>> I thought about it.. but not like we are running out of room in
>>>> semantic namespace, and it was a very straightforward way to hook this
>>>> all up (including tgsi_dump() and tgsi_text_translate())
>>>
>>>
>>> OK. I guess the main thing is that some driver author may be fooled
>>> into thinking that they should be implementing this somehow when they
>>> totally don't have to. However documentation can solve that.
>>>
>>
>> yeah, since it is just intended as a way to lower alphatest (and I
>> suspect Eric may want to do a similar thing for window w/h to deal w/
>> RECT textures), we can just define that it will only appear as a
>> non-UBO CONST, etc..
>>
>> at the end of the day, I can't dream up any hw that would need to
>> lower this sort of thing yet wants the injected constants to be
>> anything other than a CONST ;-)
>
>
> Hi Rob,
>
> We've implemented a few things like this in our driver (in-house).  How
> about passing the desired location of the alpha ref constant as a parameter
> in the tgsi_lowering_config?  So, you'd be specifying three pieces of info:
> 1. lower_alpha_test flag
> 2. the comparison function
> 3. the constant slot to find the reference value.

yeah, that does sound a bit more logical.. although the way things are
currently laid out it would involve doing an extra tgsi_scan_shader()
pass for the driver to figure out what const slot to use before
calling tgsi_lowering.  But I guess there aren't so many tgsi_lowering
users at the moment that I couldn't just juggle things around to avoid
that.

On the plus side it seems like it would be easier for the driver to
pack various params into vec4's that way..

BR,
-R

> It's often the case (at least for us) that we have to construct several
> extra constants (ex: alpha ref, texture dimensions, window height) for the
> fragment shader and the driver needs to specify which constant locations
> contain which extra values.
>
> It seems kind of odd to me to add a new semantic just for this.
>
> -Brian
>