[Intel-gfx] [PATCH v6 09/10] drm/framebuffer/tgl: Format modifier for Intel Gen 12 render compression with Clear Color

[Chery, Nanley G]

> From: Sripada, Radhakrishna
> Sent: Friday, November 01, 2019 12:00 AM
> To: Chery, Nanley G; intel-gfx at lists.freedesktop.org
> Cc: Pandiyan, Dhinakaran; Syrjala, Ville; Sharma, Shashank; Antognolli, Rafael; Ville Syrjala; Ekstrand, Jason
> Subject: RE: [PATCH v6 09/10] drm/framebuffer/tgl: Format modifier for Intel Gen 12 render compression with Clear Color
> 
> Hi Nanley,
> 
> > -----Original Message-----
> > From: Chery, Nanley G
> > Sent: Tuesday, October 29, 2019 5:05 PM
> > To: Sripada, Radhakrishna <radhakrishna.sripada at intel.com>; intel-
> > gfx at lists.freedesktop.org
> > Cc: Pandiyan, Dhinakaran <dhinakaran.pandiyan at intel.com>; Syrjala, Ville
> > <ville.syrjala at intel.com>; Sharma, Shashank
> > <shashank.sharma at intel.com>; Antognolli, Rafael
> > <rafael.antognolli at intel.com>; Ville Syrjala <ville.syrjala at linux.intel.com>;
> > Ekstrand, Jason <jason.ekstrand at intel.com>
> > Subject: RE: [PATCH v6 09/10] drm/framebuffer/tgl: Format modifier for Intel
> > Gen 12 render compression with Clear Color
> >
> > +Jason
> >
> > Maybe Jason and/or others have some thoughts on the following? Given the
> > detailed description of the clear color struct, it seems like we'll have to define
> > a new modifier if the struct fields change in a future generation.
> >
> > On negative is that we might have to make new modifiers that provide no
> > additional benefit (assuming the new/changed fields are unused). On
> > positive is that it's probably a good thing that we mentioned the Raw clear
> > color fields because I think 3D uses it during rendering operations and we'll
> > be sharing from 3D-to-3D. Maybe we should specify how the channels
> > should be formatted?
> >
> > I haven't thought through how fields like Color Discard Enable affect buffer
> > sharing...
> >
> > Another comment: I noticed that none of the Y+CCS modifiers explicitly state
> > that the CCS must be in the same BO as the Y-tiled main surface. We should
> > at least fix that for newly defined modifiers right?
> I have not tried to use separate bo for the different surfaces. That is probably worth a try.
> I wonder if we are using that for any of the modifiers to have an explicit comment?

I didn't think it was possible to use a separate BO for the aux data. AFAICT
the location of the aux data is specified as an offset from the main surface
using the PLANE_AUX_DIST register.

-Nanley

> 
> Thanks,
> RK
> >
> > -Nanley
> >
> > > ________________________________________
> > > From: Sripada, Radhakrishna
> > > Sent: Monday, October 28, 2019 1:40 PM
> > > To: intel-gfx at lists.freedesktop.org
> > > Cc: Pandiyan, Dhinakaran; Syrjala, Ville; Sharma, Shashank;
> > > Antognolli, Rafael; Chery, Nanley G; Sripada, Radhakrishna; Ville
> > > Syrjala; Kondapally, Kalyan
> > > Subject: [PATCH v6 09/10] drm/framebuffer/tgl: Format modifier for
> > > Intel Gen 12 render compression with Clear Color
> > >
> > > Gen12 display can decompress surfaces compressed by render engine with
> > > Clear Color, add a new modifier as the driver needs to know the
> > > surface was compressed by render engine.
> > >
> > > V2: Description changes as suggested by Rafael.
> > > V3: Mention the Clear Color size of 64 bits in the comments(DK)
> > > v4: Fix trailing whitespaces
> > > v5: Explain Clear Color in the documentation.
> > > v6: Documentation Nitpicks(Nanley)
> > >
> > > Cc: Ville Syrjala <ville.syrjala at linux.intel.com>
> > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan at intel.com>
> > > Cc: Kalyan Kondapally <kalyan.kondapally at intel.com>
> > > Cc: Rafael Antognolli <rafael.antognolli at intel.com>
> > > Cc: Nanley Chery <nanley.g.chery at intel.com>
> > > Signed-off-by: Radhakrishna Sripada <radhakrishna.sripada at intel.com>
> > > ---
> > >  include/uapi/drm/drm_fourcc.h | 19 +++++++++++++++++++
> > >  1 file changed, 19 insertions(+)
> > >
> > > diff --git a/include/uapi/drm/drm_fourcc.h
> > > b/include/uapi/drm/drm_fourcc.h index 1aa6d468c048..4aa7f3f9712a
> > > 100644
> > > --- a/include/uapi/drm/drm_fourcc.h
> > > +++ b/include/uapi/drm/drm_fourcc.h
> > > @@ -434,6 +434,25 @@ extern "C" {
> > >   */
> > >  #define I915_FORMAT_MOD_Y_TILED_GEN12_MC_CCS
> > fourcc_mod_code(INTEL,
> > > 7)
> > >
> > > +/*
> > > + * Intel Color Control Surface with Clear Color (CCS) for Gen-12
> > > +render
> > > + * compression.
> > > + *
> > > + * The main surface is Y-tiled and is at plane index 0 whereas CCS is
> > > +linear
> > > + * and at index 1. The clear color is stored at index 2, and the
> > > +pitch should
> > > + * be ignored. The clear color structure is 256 bits. The first 128
> > > +bits
> > > + * represents Raw Clear Color Red, Green, Blue and Alpha color each
> > > +represented
> > > + * by 32 bits. The raw clear color is consumed by the 3d engine and
> > > +generates
> > > + * the converted clear color of size 64 bits. The first 32 bits store
> > > +the Lower
> > > + * Converted Clear Color value and the next 32 bits store the Higher
> > > +Converted
> > > + * Clear Color value when applicable. The Converted Clear Color
> > > +values are
> > > + * consumed by the DE. The last 64 bits are used to store Color
> > > +Discard Enable
> > > + * and Depth Clear Value Valid which are ignored by the DE. A CCS
> > > +cache line
> > > + * corresponds to an area of 4x1 tiles in the main surface. The main
> > > +surface
> > > + * pitch is required to be a multiple of 4 tile widths.
> > > + */
> > > +#define I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC
> > > +fourcc_mod_code(INTEL, 8)
> > > +
> > >  /*
> > >   * Tiled, NV12MT, grouped in 64 (pixels) x 32 (lines) -sized macroblocks
> > >   *
> > > --
> > > 2.20.1
> > >