[Mesa-dev] [PATCH v2] i965: Fix point size with tessellation/geometry shaders in GLES.

[Kenneth Graunke]

On Friday, June 3, 2016 4:40:46 PM PDT Ilia Mirkin wrote:
> On Fri, Jun 3, 2016 at 4:08 AM, Kenneth Graunke <kenneth at whitecape.org> wrote:
> > Our previous code worked for desktop GL, and ES without geometry or
> > tessellation shaders.  But those features require fancier point size
> > handling.  Fortunately, we can use one rule for all APIs.
> >
> > Fixes a number of dEQP tests with EXT_tessellation_shader enabled:
> > dEQP-GLES31.functional.tessellation_geometry_interaction.point_size.*
> >
> > Cc: Ilia Mirkin <imirkin at alum.mit.edu>
> > Signed-off-by: Kenneth Graunke <kenneth at whitecape.org>
> > ---
> >  src/mesa/drivers/dri/i965/brw_state.h     | 49 +++++++++++++++++++++++++++++++
> >  src/mesa/drivers/dri/i965/gen6_sf_state.c |  5 ++--
> >  src/mesa/drivers/dri/i965/gen7_sf_state.c |  7 +++--
> >  src/mesa/drivers/dri/i965/gen8_sf_state.c |  7 +++--
> >  4 files changed, 59 insertions(+), 9 deletions(-)
> >
> > Hey Ilia,
> >
> > Thanks for helping me figure out these rules on IRC yesterday.
> > I think this version should work...
> >
> >  --Ken
> >
> > diff --git a/src/mesa/drivers/dri/i965/brw_state.h b/src/mesa/drivers/dri/i965/brw_state.h
> > index 0a4c21f..be7f6ce 100644
> > --- a/src/mesa/drivers/dri/i965/brw_state.h
> > +++ b/src/mesa/drivers/dri/i965/brw_state.h
> > @@ -473,6 +473,55 @@ is_drawing_lines(const struct brw_context *brw)
> >     return false;
> >  }
> >
> > +static inline bool
> > +use_state_point_size(const struct brw_context *brw)
> > +{
> > +   const struct gl_context *ctx = &brw->ctx;
> > +
> > +   /* Section 14.4 (Points) of the OpenGL 4.5 specification says:
> > +    *
> > +    *    "If program point size mode is enabled, the derived point size is
> > +    *     taken from the (potentially clipped) shader built-in gl_PointSize
> > +    *     written by:
> > +    *
> > +    *        * the geometry shader, if active;
> > +    *        * the tessellation evaluation shader, if active and no
> > +    *          geometry shader is active;
> > +    *        * the vertex shader, otherwise
> > +    *
> > +    *    and clamped to the implementation-dependent point size range.  If
> > +    *    the value written to gl_PointSize is less than or equal to zero,
> > +    *    or if no value was written to gl_PointSize, results are undefined.
> > +    *    If program point size mode is disabled, the derived point size is
> > +    *    specified with the command
> > +    *
> > +    *       void PointSize(float size);
> > +    *
> > +    *    size specifies the requested size of a point.  The default value
> > +    *    is 1.0."
> > +    *
> > +    * The rules for GLES come from the ES 3.2, OES_geometry_point_size, and
> > +    * OES_tessellation_point_size specifications.  To summarize: if the last
> > +    * stage before rasterization is a GS or TES, then use gl_PointSize from
> > +    * the shader if written.  Otherwise, use 1.0.  If the last stage is a
> > +    * vertex shader, use gl_PointSize, or it is undefined.
> > +    *
> > +    * We can combine these rules into a single condition for both APIs.
> > +    * Using the state point size when the last shader stage doesn't write
> > +    * gl_PointSize satisfies GL's requirements, as it's undefined.  Because
> > +    * ES doesn't have a PointSize() command, the state point size will
> > +    * remain 1.0, satisfying the ES default value in the GS/TES case, and
> > +    * the VS case (1.0 works for "undefined").  Mesa sets the program point
> > +    * mode flag to always-enabled in ES, so we can safely check that, and
> > +    * it'll be ignored for ES.
> > +    *
> > +    * _NEW_PROGRAM | _NEW_POINT
> > +    * BRW_NEW_VUE_MAP_GEOM_OUT
> > +    */
> > +   return (!ctx->VertexProgram.PointSizeEnabled && !ctx->Point._Attenuated) ||
> > +          (brw->vue_map_geom_out.slots_valid & VARYING_BIT_PSIZ) == 0;
> 
> I believe that this is "overly" nice to buggy programs, but not
> incorrect. The cases where this has to return true:
> 
> (a) point size is explicitly disabled
> (b) ES2 context, and point size isn't emitted in TES/GS
> 
> And it has to return false when point size is enabled && "last stage"
> emits a point size. In all other cases, you can do whatever. I believe
> you're choosing to be nice and use the fixed function point size if a
> point size isn't being emitted, which is fine but unnecessary.

Right.  I decided to use the API-specified point size in a few undefined
cases because I figured it would actually take more code to be meaner.

> Dunno what the _Attenuated thing is, but it was like that before too,
> so I assume you know what you're doing.

Apparently there's a GL_DISTANCE_ATTENUATION_EXT point parameter, where
you can set three values, and it varies the point size based on the
distance.  We emulate this in ffvertex_prog.c.

> In principle this makes sense to me, but I don't know what
> vue_map_geom_out actually is, so I'm going to leave the actual review
> to someone who knows the driver better.
> 
>   -ilia

vue_map_geom_out is just a data structure representing the outputs of
the last stage enabled in the pipeline.  brw_state_upload.c's
brw_upload_programs function sets it very similarly to what you did in
st/mesa.  slots_valid is basically just OutputsWritten in this case.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part.
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20160605/d682daf9/attachment.sig>