[Mesa-dev] [PATCH 01/13] i965: Allow stencil to be used for sampling and as render target

[Pohjolainen, Topi]

On Fri, May 09, 2014 at 05:07:44PM -0700, Kenneth Graunke wrote:
> On 05/09/2014 01:28 AM, Topi Pohjolainen wrote:
> > Signed-off-by: Topi Pohjolainen <topi.pohjolainen at intel.com>
> > ---
> >  src/mesa/drivers/dri/i965/brw_surface_formats.c | 7 +++++--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> > diff --git a/src/mesa/drivers/dri/i965/brw_surface_formats.c b/src/mesa/drivers/dri/i965/brw_surface_formats.c
> > index 5907dd9..7ffa5c4 100644
> > --- a/src/mesa/drivers/dri/i965/brw_surface_formats.c
> > +++ b/src/mesa/drivers/dri/i965/brw_surface_formats.c
> > @@ -221,7 +221,7 @@ const struct surface_format_info surface_formats[] = {
> >     SF( Y,  Y,  x, 45,  Y,  Y,  Y,  x,  x, BRW_SURFACEFORMAT_R8_UNORM)
> >     SF( Y,  Y,  x,  x,  Y, 60,  Y,  x,  x, BRW_SURFACEFORMAT_R8_SNORM)
> >     SF( Y,  x,  x,  x,  Y,  x,  Y,  x,  x, BRW_SURFACEFORMAT_R8_SINT)
> > -   SF( Y,  x,  x,  x,  Y,  x,  Y,  x,  x, BRW_SURFACEFORMAT_R8_UINT)
> > +   SF(60,  x,  x,  x, 60,  x,  Y,  x,  x, BRW_SURFACEFORMAT_R8_UINT)
> 
> This hunk seems wrong to me.  From the G45 PRM, Volume 4, page 133:
> 
> | Y |   |   |   | Y |   | Y |   |   | R8_UINT
> 
> So, AFAICT, this has been supported since the original Broadwater/G965.
> 
> >     SF( Y,  Y,  x,  Y,  Y,  Y,  x,  x,  x, BRW_SURFACEFORMAT_A8_UNORM)
> >     SF( Y,  Y,  x,  x,  x,  x,  x,  x,  x, BRW_SURFACEFORMAT_I8_UNORM)
> >     SF( Y,  Y,  x,  Y,  x,  x,  x,  x,  x, BRW_SURFACEFORMAT_L8_UNORM)
> > @@ -594,9 +594,12 @@ brw_init_surface_formats(struct brw_context *brw)
> >         * integer, so we don't need hardware support for blending on it.  Other
> >         * than that, GL in general requires alpha blending for render targets,
> >         * even though we don't support it for some formats.
> > +       * Stencil is also supported as render targer for internal blitting and
> 
> render target (typo)
> 
> > +       * scaling purposes.
> >         */
> >        if (gen >= rinfo->render_target &&
> > -	  (gen >= rinfo->alpha_blend || is_integer)) {
> > +	  (gen >= rinfo->alpha_blend || is_integer ||
> > +           format == MESA_FORMAT_S_UINT8)) {
> >  	 brw->render_target_format[format] = render;
> >  	 brw->format_supported_as_render_target[format] = true;
> >        }
> 
> I noticed there is already code that does:
> 
>    brw->format_supported_as_render_target[MESA_FORMAT_S_UINT8] = true;
> 
> Your patch additionally causes:
> 
>    brw->render_target_format[MESA_FORMAT_S_UINT8] =
> BRW_SURFACEFORMAT_R8_UINT;
> 
> which seems reasonable.  However, I don't think any of your patches
> actually rely on this fact, since you override the format in
> brw_configure_w_tiled.  So, perhaps this patch can be dropped?
> 

This is in fact misleading, it only overrides it for texture surface path. In
case of render target the override gotten from brw_configure_w_tiled() is
ignored:

   /* _NEW_BUFFERS */
   mesa_format rb_format = _mesa_get_render_format(ctx, intel_rb_format(irb));
   assert(brw_render_target_supported(brw, rb));
   format = brw->render_target_format[rb_format];


And here the lookup needs the additional change in the table. I didn't like
this patch in the first place but I kind of sticked to it because there were
so much other work related with stencil in SNB/IVB front.

I would rather move the "brw_configure_w_tiled()" after the normal format
resolve and let it override the format for render target as well. How would you
like that?

-Topi