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.pohjolai...@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 _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev