[Mesa-dev] [PATCH 1/2] i965/gen9: Annotate input coverage mask change

[Kenneth Graunke]

On Thursday, August 27, 2015 11:50:51 AM Ben Widawsky wrote:
> As far as I can tell, the behavior is preserved from the previous generations.
> Before we set a single bit to tell the FS whether or not we'll be using an input
> coverage mask. Now we have some options which are implementing various
> extensions. These bits are used for the various conservative rasterization
> mechanisms (for collision detection, binning, and whatever else).
> 
> I believe that the behavior is preserved because the problem which conservative
> rasterization is attempting to fix would go away with the "NORMAL" mode (at the
> cost of performance, I believe).
> 
> This patch serves as documentation of the change by creating the enums, as well
> as giving some of the history with the links here so that the next person who
> comes along and looks at it doesn't spend as long as I had to in order to
> determine if there is an issue or not.
> 
> Previously, this algorithm had been done in software, and this can still be used
> as long as we don't export an extension stating otherwise.
> 
> References: https://developer.nvidia.com/sites/default/files/akamai/opengl/specs/GL_NV_conservative_raster.txt

That's an outdated version of the extension spec, try this instead:

References: https://www.opengl.org/registry/specs/NV/conservative_raster.txt

> References: https://http.developer.nvidia.com/GPUGems2/gpugems2_chapter42.html
> Cc: Kristian Høgsberg <krh at bitplanet.net>
> Signed-off-by: Ben Widawsky <ben at bwidawsk.net>
> ---
>  src/mesa/drivers/dri/i965/brw_defines.h   | 16 ++++++++++++++++
>  src/mesa/drivers/dri/i965/gen8_ps_state.c |  8 ++++++--
>  2 files changed, 22 insertions(+), 2 deletions(-)
> 
> diff --git a/src/mesa/drivers/dri/i965/brw_defines.h b/src/mesa/drivers/dri/i965/brw_defines.h
> index cb5c82a..a9c1e08 100644
> --- a/src/mesa/drivers/dri/i965/brw_defines.h
> +++ b/src/mesa/drivers/dri/i965/brw_defines.h
> @@ -2265,6 +2265,21 @@ enum brw_pixel_shader_computed_depth_mode {
>     BRW_PSCDEPTH_ON_LE = 3, /* PS guarantees output depth <= source depth */
>  };
>  
> +enum brw_pixel_shader_coverage_mask_mode {
> +   BRW_PSICMS_OFF     = 0, /* PS does not use input coverage masks. */
> +   BRW_PSICMS_NORMAL  = 1, /* Input Coverage masks based on outer conservatism
> +                            * and factors in SAMPLE_MASK.  If Pixel is
> +                            * conservatively covered, all samples are enabled.
> +                            */
> +
> +   BRW_PSICMS_INNER   = 2, /* Input Coverage masks based on inner conservatism
> +                            * and factors in SAMPLE_MASK.  If Pixel is
> +                            * conservatively *FULLY* covered, all samples are
> +                            * enabled.
> +                            */
> +   BRW_PCICMS_DEPTH   = 3,
> +};
> +
>  #define _3DSTATE_PS_EXTRA                       0x784F /* GEN8+ */
>  /* DW1 */
>  # define GEN8_PSX_PIXEL_SHADER_VALID                    (1 << 31)
> @@ -2282,6 +2297,7 @@ enum brw_pixel_shader_computed_depth_mode {
>  # define GEN9_PSX_SHADER_PULLS_BARY                     (1 << 3)
>  # define GEN8_PSX_SHADER_HAS_UAV                        (1 << 2)
>  # define GEN8_PSX_SHADER_USES_INPUT_COVERAGE_MASK       (1 << 1)
> +# define GEN9_PSX_SHADER_NORMAL_COVERAGE_MASK_SHIFT     0
>  
>  enum brw_wm_barycentric_interp_mode {
>     BRW_WM_PERSPECTIVE_PIXEL_BARYCENTRIC		= 0,
> diff --git a/src/mesa/drivers/dri/i965/gen8_ps_state.c b/src/mesa/drivers/dri/i965/gen8_ps_state.c
> index ae18f0f..a686fed 100644
> --- a/src/mesa/drivers/dri/i965/gen8_ps_state.c
> +++ b/src/mesa/drivers/dri/i965/gen8_ps_state.c
> @@ -52,8 +52,12 @@ gen8_upload_ps_extra(struct brw_context *brw,
>         _mesa_get_min_invocations_per_fragment(ctx, fp, false) > 1)
>        dw1 |= GEN8_PSX_SHADER_IS_PER_SAMPLE;
>  
> -   if (fp->Base.SystemValuesRead & SYSTEM_BIT_SAMPLE_MASK_IN)
> -      dw1 |= GEN8_PSX_SHADER_USES_INPUT_COVERAGE_MASK;
> +   if (fp->Base.SystemValuesRead & SYSTEM_BIT_SAMPLE_MASK_IN) {
> +      if (brw->gen >= 9)
> +         dw1 |= BRW_PSICMS_INNER << GEN9_PSX_SHADER_NORMAL_COVERAGE_MASK_SHIFT;
> +      else
> +         dw1 |= GEN8_PSX_SHADER_USES_INPUT_COVERAGE_MASK;

The documentation on this is exceedingly scarce, but I believe you're
right, and "INNER" corresponds to the traditional mode.  It sounds like
it corresponds to the underestimating mode described in the paper you
cited, and "NORMAL" corresponds to the overestimating mode, where any
sample covered => all enabled.

It's also the same bit (2 << 0 as opposed to 1 << 1), which suggests
it's probably equivalent.

This looks good to me - it's certainly confusing, and this clarifies the
situation.  Thanks for investigating, Ben - nice work :)

With the URL changed,
Reviewed-by: Kenneth Graunke <kenneth at whitecape.org>

We might want to add GL_NV_conservative_render to the I965Todo page...

> +   }
>  
>     if (prog_data->uses_omask)
>        dw1 |= GEN8_PSX_OMASK_TO_RENDER_TARGET;
> 
-------------- 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: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20150827/b3ec9b62/attachment.sig>