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

[Alex Deucher]

On Tue, Nov 9, 2010 at 12:05 PM, Roland Scheidegger <sroland at vmware.com> wrote:
> 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.

Good enough for me.

Alex

>
> 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
>>>
>
>