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

[Chery, Nanley G]

> From: Chery, Nanley G
> Sent: Tuesday, November 12, 2019 9:40 AM
> To: Sripada, Radhakrishna; 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
> 
> > 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...
> > >

After some more thought, I don't think it's a problem to have a detailed
description here. We have a lot of space allocated for making new modifiers
(and the header suggests doing so!).

We could go into more detail about the non-DE-specific fields, but I suppose
users can access PRMs.

> > > 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.
> 

Great, I got some clarification that this is actually possible. 

Lastly, can we replace: 

* The raw clear color is consumed by the 3d engine and generates
* the converted clear color of size 64 bits.

with something like:

* The 3D engine can use the raw clear color and the surface format to
* generate a converted clear color of size 64 bits.

so that the subject is constant throughout the sentence and so that it's a bit
more obvious what is in the Converted Clear Color field? Also, we don't
explicitly state that the Converted Clear Color comes right after the raw clear
color, but I'd think the implication is strong enough.

With the above suggestion implemented, this patch is
Reviewed-by: Nanley Chery <nanley.g.chery at intel.com>

-Nanley

> -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
> > > >