[Mesa-dev] [PATCH 4/9] r200: Enable extensions by just setting the flags

[Roland Scheidegger]

Am 08.09.2011 21:43, schrieb Ian Romanick:
> On 09/08/2011 12:18 PM, Roland Scheidegger wrote:
>> Am 08.09.2011 21:03, schrieb Roland Scheidegger:
>>> Am 08.09.2011 19:53, schrieb Ian Romanick:
>>>> On 09/06/2011 03:21 PM, Roland Scheidegger wrote:
>>>>> Am 06.09.2011 22:13, schrieb Ian Romanick:
>>>>>> From: Ian Romanick <ian.d.romanick at intel.com>
>>>>>>
>>>>>> Core Mesa already does the dispatch offset remapping for
>>>>>> every function that could possibly ever be supported.
>>>>>> There's no need to continue using that cruft in the
>>>>>> driver.
>>>>>>
>>>>>> Since the call to _mesa_enable_imaging_extensions (via 
>>>>>> driInitExtensions) is removed, EXT_blend_logic_op is
>>>>>> explicitly added to the list.  EXT_blend_color is also
>>>>>> added, but it depends on the drmSupportsBlendColor flag.
>>>>
>>>>> Hmm, I don't think EXT_blend_logic_op was advertized before.
>>>>> The reason for this is that EXT_blend_logic_op together with
>>>>
>>>> EXT_blend_logic_op *was* previously enabled.  r200CreateContext
>>>> called driInitExtensions( ctx, card_extensions, GL_TRUE );.
>>>> The GL_TRUE parameter tells driInitExtensions to call 
>>>> _mesa_enable_imaging_extensions.
>>>> _mesa_enable_imaging_extensions in turn enables:
>>>>
>>>> GL_EXT_blend_color GL_EXT_blend_logic_op GL_EXT_blend_minmax 
>>>> GL_EXT_blend_subtract
>>> That's right. _mesa_enable_imaging_extensions however did not
>>> always enable EXT_blend_logic_op. I was curious (as I was sure it
>>> was correct at one point for r200) when this got broken for r200
>>> and that's the answer: 6775c1e8ccc2c543d97eb273a342140a62d99aee -
>>> that is OLD (interestingly the commit mentions some discussion
>>> apparently however I did not realize it in fact made r200
>>> advertize EXT_blend_logic_op which I knew would be incorrect).
>>>
>>>>
>>>> I didn't see anything in r200_state.c to handle blend equation
>>>> being set to GL_LOGIC_OP.
>>> Yes - there was code initially handling this (trivial as long as
>>> it's the same on all RGBA channels) but I removed that a decade
>>> or so ago when adding support for EXT_blend_equation_separate
>>> (and removing support for EXT_blend_logic_op at the same time).
>>>
>>>>
>>>> Of course, we have *zero* piglit tests for this extension.
>>>>
>>>>> EXT_blend_equation_separate allows some unholy combinations
>>>>> which the r200 (possibly other hw too) can't handle
>>>>> correctly. Namely this combination makes it possible to have
>>>>> logic ops on rgb or alpha channels and color blending on the
>>>>> other channels. I know that at least sometime in the past
>>>>> this driver did not advertize EXT_blend_logic_op, since
>>>>> OpenGL 1.1 style logic ops do not have that problem and
>>>>> EXT_blend_logic_op wasn't really all that important. I guess 
>>>>> though it's not exactly a severe problem since surely apps
>>>>> old enough to use EXT_blend_logic_op wouldn't try to use
>>>>> EXT_blend_equation_separate (though in theory some app could
>>>>> be clever and really want to do that...).
>>>>
>>>> That's a good point.  I suspect that no hardware actually
>>>> handles this case correctly.  I seem to recall that this is the
>>>> reason NVIDIA doesn't support GL_EXT_blend_logic_op in their
>>>> drivers.  I know the non-Quadro cards don't support it,
>>>> anyway.
>>>>
>>>> Does this work on later chips in the Radeon family?
>>>>
>>>> I don't think anyone will miss GL_EXT_blend_logic_op if we just
>>>> remove it altogether.
>>>
>>> I don't think it works at least for r300. FWIW there's a mesa
>>> helper function (_mesa_rgba_logicop_enabled) which also only
>>> makes sense if the logicop blend equation is set for either both
>>> of none of RGB/A.
>>>
>>> I certainly wouldn't mourn the loss of EXT_blend_logic_op.
> 
> 
>> Hmm actually a quick search of ARB_color_imaging reveals that 
>> EXT_blend_logic_op isn't even part of it (not that it matters much
>> as we don't support the imaging subset any longer anyway) so I
>> don't know why it got enabled there. In any case since that imaging
>> subset enable is gone, it's not really a problem anymore and
>> drivers can just enable it if they really want - r200 certainly
>> does not.
> 
> Okay.  I'll disable it in r200 and r300.  With that change, do I have
> your Reviewed-by?

Yes otherwise looks good.
Given that r300 (but not r200) actually can handle logicop blend
equation (just not with separate logicop blend equation) and the fact
that mesa due to incorrect implementation of EXT_blend_logic_op actually
will never do separate logicop blend equation I don't know what the
correct answer there is. Looks like 3 ways possible:
1) leave the bogus mesa logicop check in and hence drivers like r300 can
still claim support for EXT_blend_logic_op. This is incorrect wrt to
spec but might allow some REALLY old apps requiring this extension to
run (could also reimplement it for r200 if anyone is interested...)
2) remove EXT_blend_logic_op completely.
3) fix mesa EXT_blend_logic_op/EXT_blend_equation_separate interaction.
This probably means most (all?) drivers can't actually claim
EXT_blend_logic_op support with a straight face. (Not that with solution
1) that's really different...)

Either way looks ok to me.


Reviewed-by: Roland Scheidegger <sroland at vmware.com>