[Mesa-dev] [PATCH 2/7] i965: Add lossless compression to surface format table
Chad Versace
chad.versace at intel.com
Mon Nov 16 12:29:58 PST 2015
On Mon 16 Nov 2015, Ben Widawsky wrote:
> On Fri, Nov 13, 2015 at 12:29:47PM -0800, Chad Versace wrote:
> > On Wed 11 Nov 2015, Ben Widawsky wrote:
> > > Background: Prior to Skylake and since Ivybridge Intel hardware has had the
> > > ability to use a MCS (Multisample Control Surface) as auxiliary data in
> > > "compression" operations on the surface. This reduces memory bandwidth. This
> > > hardware was either used for MSAA compression, and fast clear operations. On
> > > Gen8, a similar mechanism exists to allow the hiz buffer to be sampled from, and
> > > therefore this feature is sometimes referred to more generally as "AUX buffers".
> > >
> > > Skylake adds the ability to have the display engine directly source compressed
> > > surfaces on top of the ability to sample from them. Inference dictates that
> > > enabling this display features adding a restriction to the formats which could
> > > actually be compressed. The current set of surfaces seems to be a subset as
> > > compared to previous gens (see the next patch). Also, if I had to guess I would
> > > guess that future gens add support for more surface formats. To make handling
> > > this a bit easier to read, and more future proof, the support for this is moved
> > > into the surface formats table.
> > >
> > > Along with the modifications to the table, a helper function is also provided to
> > > determine if a surface is CCS compatible. Because fast clears are currently
> > > disabled on SKL, we can plumb the helper all the way through here, and not
> > > actually have anything break.
> > >
> > > The logic in the table works a bit differently than the other columns in the
> > > table and therefore deserves a small mention. For most other features, the GEN
> > > which began implementing it is set, and it is assumed future gens also support
> > > this. For this feature, GEN9 actually eliminates support for certain formats. We
> > > could use this column to determine support for the similar feature on older
> > > generation hardware. Aside from that being an error prone task which is
> > > unrelated to enabling this on GEN9, it becomes somewhat tricky to implement
> > > because of the fact that surface format support diminishes. You'd probably want
> > > another column to cleanly implement it.
> > >
> > > Requested-by: Chad Versace <chad.versace at intel.com>
> > > Requested-by: Neil Roberts <neil at linux.intel.com>
> > > Signed-off-by: Ben Widawsky <benjamin.widawsk at intel.com>
> > > ---
> > > src/mesa/drivers/dri/i965/brw_context.h | 2 +
> > > src/mesa/drivers/dri/i965/brw_surface_formats.c | 527 +++++++++++++-----------
> > > src/mesa/drivers/dri/i965/intel_mipmap_tree.c | 7 +
> > > 3 files changed, 285 insertions(+), 251 deletions(-)
> > >
> > > diff --git a/src/mesa/drivers/dri/i965/brw_context.h b/src/mesa/drivers/dri/i965/brw_context.h
> > > index 4b2db61..6284c18 100644
> > > --- a/src/mesa/drivers/dri/i965/brw_context.h
> > > +++ b/src/mesa/drivers/dri/i965/brw_context.h
> > > @@ -1465,6 +1465,8 @@ void brw_upload_image_surfaces(struct brw_context *brw,
> > > /* brw_surface_formats.c */
> > > bool brw_render_target_supported(struct brw_context *brw,
> > > struct gl_renderbuffer *rb);
> > > +bool brw_losslessly_compressible_format(struct brw_context *brw,
> > > + uint32_t brw_format);
> > > uint32_t brw_depth_format(struct brw_context *brw, mesa_format format);
> > > mesa_format brw_lower_mesa_image_format(const struct brw_device_info *devinfo,
> > > mesa_format format);
> > > diff --git a/src/mesa/drivers/dri/i965/brw_surface_formats.c b/src/mesa/drivers/dri/i965/brw_surface_formats.c
> > > index 97fff60..a7cdc13 100644
> > > --- a/src/mesa/drivers/dri/i965/brw_surface_formats.c
> > > +++ b/src/mesa/drivers/dri/i965/brw_surface_formats.c
> > > @@ -39,14 +39,15 @@ struct surface_format_info {
> > > int input_vb;
> > > int streamed_output_vb;
> > > int color_processing;
> > > + int lossless_compression_support;
> >
> > There's no need to place "support" in the name. Every struct member is
> > a "support" member.
> >
>
> Fine.
>
> > > const char *name;
> > > };
> > >
> > > /* This macro allows us to write the table almost as it appears in the PRM,
> > > * while restructuring it to turn it into the C code we want.
> > > */
> > > -#define SF(sampl, filt, shad, ck, rt, ab, vb, so, color, sf) \
> > > - [BRW_SURFACEFORMAT_##sf] = { true, sampl, filt, shad, ck, rt, ab, vb, so, color, #sf},
> > > +#define SF(sampl, filt, shad, ck, rt, ab, vb, so, color, ccs, sf) \
> > > + [BRW_SURFACEFORMAT_##sf] = { true, sampl, filt, shad, ck, rt, ab, vb, so, color, ccs, #sf},
> > >
> > > #define Y 0
> > > #define x 999
> > > @@ -74,6 +75,7 @@ struct surface_format_info {
> > > * VB - Input Vertex Buffer
> > > * SO - Steamed Output Vertex Buffers (transform feedback)
> > > * color - Color Processing
> > > + * ccs - Lossless Compression Support (gen9+ only)
> >
> > Please don't name the new column "ccs". That conflates two closely
> > related but distinct hardware features.
> >
> > According the internal hw docs, Broadwell supports AUX_CCS_D (legacy
> > "clear compression", my personal term), but Broadwell most definitely
> > does not support AUX_CCS_E (lossless color compression). In other words,
> > Broadwell supports CCS but not lossless color compression.
> >
>
> I am sorry you found it confusing. Apparently you are confused by all the code I
> write... what would you like me to name it? I'm done with trying to predict how
> you would have written the patches. Going for on this patch series, just tell me
> what you prefer so we can avoid the excessive back and forth.
Does naming it either 'ccs_e' or 'lossless_compression' work for you?
I prefer 'ccs_e' because it's less ambiguous and shorter than
'lossless_compression', but either would work.
It's just that the member name needs to distinguish between 'ccs_d' and
'ccs_e'.
> > > + * True if the underlying hardware format can support lossless color compression
> > > + * (MSAA compression, or Color Compression for render targets).
> >
> > The comment needs fixing. "MSAA compression" is not "lossless color
> > compression". Skylake supports lossless color compression is only for
> > single-sample surfaces.
> >
> > I think the comment should end with a period after "lossless color
> > compression". That comment would suffice and be correct.
> >
>
> Okay.
>
> > > + */
> > > +bool
> > > +brw_losslessly_compressible_format(struct brw_context *brw,
> > > + uint32_t brw_format)
> > > +{
> > > + const struct surface_format_info *sinfo;
> > > + int gen = brw->gen * 10;
> > > +
> > > + /* Prior to gen9, there are no restrictions */
> > > + if (brw->gen < 9)
> > > + return true;
> >
> > The pre-gen9 check is wrong. It should return false. Lossless color
> > compression does not exist before gen9.
> >
> > Anyway, after you fill the lossless_compression column in the
> > surface_formats table in the next patch, the 'if' statement above can be
> > completely removed. The 'if' statement below suffices.
> >
>
> Essentially you're asking me to invert the logic in the table iirc. That should
> be fine, there was some reason I didn't do this in the first place, but I can't
> think of what it was now. I'll make the change after you respond to the above.
There's no need to repeat the discussion we had on IRC. I believe Ken's
suggestions are good ways forward.
12:20 <Kayden> bwidawsk: so sounds like you have two ways
forward - either move the gen checks around like I suggested (so the
function is just for CCS_E), or rename the function to
brw_is_clear_compressible_format() or
brw_format_supports_fast_clear()
More information about the mesa-dev
mailing list