[Mesa-dev] [PATCH 1/2] r200: fix r200 large points

[Roland Scheidegger]

On 09.11.2010 17:17, Alex Deucher wrote:
> On Mon, Nov 8, 2010 at 4:12 PM, Roland Scheidegger <sroland at vmware.com> wrote:
>> DD_POINT_SIZE got never set for some time now (as it was set only in ifdefed
>> out code), which caused the r200 driver to use the point primitive mistakenly
>> in some cases which can only do size 1 instead of point sprite. Since the
>> logic to use point instead of point sprite prim is flaky at best anyway (can't
>> work correctly for per-vertex point size), just drop this and always emit point
>> sprites (except for AA points) - reasons why the driver tried to use points for
>> size 1.0 are unknown though it is possible they are faster or more conformant.
>> Note that we can't emit point sprites without point sprite cntl as that might
>> result in undefined point sizes, hence need drm version check (which was
>> unnecessary before as it should always have selected points). An
>> alternative would be to rely on the RE point size clamp controls which could
>> clamp the size to 1.0 min/max even if the SE point size is undefined, but currently
>> always use 0 for min clamp. (As a side note, this also means the driver does
>> not honor the gl spec which mandates points, but not point sprites, with zero size
>> to be rendered as size 1.)
>> This should fix recent reports of https://bugs.freedesktop.org/show_bug.cgi?id=702.
>> This is a candidate for the mesa 7.9 branch.
> 
> Looks good to me.  I suppose a drm cs checker fix would be nice for
> completeness, but as long as the 3D driver emits the point sprite cntl
> properly, I think it should be fine.  Are there any cases where it
> wouldn't?
No, it always should (with new enough drm - from 2004 or so...)
I don't think drm cs check is necessary for that. If the point sprite
cntl reg isn't emitted, I guess the chip might do bogus and unnecessary
point size calculations (since the parameters used were never set, as
well as coord replacement might happen or not). The regs don't have a
default value so when assuming it's zero the chip unfortunately will not
use PS_SE_SEL_STATE_SIZE hence use some random value instead of the
point size set in the RE reg (I quickly tried that out and it resulted
indeed in pretty much random size - the docs say it will use per-vertex
point size "if present", no word on what happens if it's not present).
But all of that should be perfectly safe I think, and it probably would
be quite difficult to track correctly anyway.

Roland


> 
> Reviewed-by: Alex Deucher <alexdeucher at gmail.com>
> 
>> ---
>>  src/mesa/drivers/dri/r200/r200_swtcl.c |    7 +++----
>>  src/mesa/drivers/dri/r200/r200_tcl.c   |    5 ++---
>>  2 files changed, 5 insertions(+), 7 deletions(-)
>>
>> diff --git a/src/mesa/drivers/dri/r200/r200_swtcl.c b/src/mesa/drivers/dri/r200/r200_swtcl.c
>> index 3886416..c56a49d 100644
>> --- a/src/mesa/drivers/dri/r200/r200_swtcl.c
>> +++ b/src/mesa/drivers/dri/r200/r200_swtcl.c
>> @@ -319,10 +319,9 @@ static INLINE GLuint reduced_hw_prim( struct gl_context *ctx, GLuint prim)
>>  {
>>    switch (prim) {
>>    case GL_POINTS:
>> -      return (ctx->Point.PointSprite ||
>> -        ((ctx->_TriangleCaps & (DD_POINT_SIZE | DD_POINT_ATTEN)) &&
>> -        !(ctx->_TriangleCaps & (DD_POINT_SMOOTH)))) ?
>> -        R200_VF_PRIM_POINT_SPRITES : R200_VF_PRIM_POINTS;
>> +      return (((R200_CONTEXT(ctx))->radeon.radeonScreen->drmSupportsPointSprites &&
>> +              !(ctx->_TriangleCaps & DD_POINT_SMOOTH)) ?
>> +        R200_VF_PRIM_POINT_SPRITES : R200_VF_PRIM_POINTS);
>>    case GL_LINES:
>>    /* fallthrough */
>>    case GL_LINE_LOOP:
>> diff --git a/src/mesa/drivers/dri/r200/r200_tcl.c b/src/mesa/drivers/dri/r200/r200_tcl.c
>> index 84db7c9..7aed116 100644
>> --- a/src/mesa/drivers/dri/r200/r200_tcl.c
>> +++ b/src/mesa/drivers/dri/r200/r200_tcl.c
>> @@ -68,9 +68,8 @@ WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
>>  #define HAVE_ELTS        1
>>
>>
>> -#define HW_POINTS           ((ctx->Point.PointSprite || \
>> -                               ((ctx->_TriangleCaps & (DD_POINT_SIZE | DD_POINT_ATTEN)) && \
>> -                               !(ctx->_TriangleCaps & (DD_POINT_SMOOTH)))) ? \
>> +#define HW_POINTS           (((R200_CONTEXT(ctx))->radeon.radeonScreen->drmSupportsPointSprites && \
>> +                             !(ctx->_TriangleCaps & DD_POINT_SMOOTH)) ? \
>>                                R200_VF_PRIM_POINT_SPRITES : R200_VF_PRIM_POINTS)
>>  #define HW_LINES            R200_VF_PRIM_LINES
>>  #define HW_LINE_LOOP        0
>> --
>> 1.7.0.4
>>
>> _______________________________________________
>> mesa-dev mailing list
>> mesa-dev at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>>