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

[Kenneth Graunke]

On 05/09/2014 09:50 PM, Pohjolainen, Topi wrote:
> 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

That could work.  Though, I think an easy solution would be to move this block
into the } else { case of the S8 check:

   if (mt->format == MESA_FORMAT_S_UINT8) {
      brw_configure_w_tiled(mt, true, &width, &height, &pitch,
                            &tiling, &format);
   } else {
      assert(mt->msaa_layout != INTEL_MSAA_LAYOUT_IMS);

      /* _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];
      if (unlikely(!brw->format_supported_as_render_target[rb_format])) {
         _mesa_problem(ctx, "%s: renderbuffer format %s unsupported\n",
                       __FUNCTION__, _mesa_get_format_name(rb_format));
      }
   }

(I ran this idea by Topi on IRC and he seemed to like it.)

On a slight tangent, I think the _mesa_problem block is pretty useless.
It should never happen.  In debug builds, we would have failed the
assert(brw_render_target_supported(brw, rb)); check anyway.  In release
builds, it plows ahead anyway and would probably hang the GPU or do
something nasty.  So it doesn't even add an extra safety check.  We
could just drop it.

--Ken

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: OpenPGP digital signature
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20140509/f0be27c4/attachment.sig>