[Intel-gfx] [PATCH 4/7] drm/i915/adlp/fb: Fix remapping of linear CCS AUX surfaces

[Imre Deak]

On Fri, Oct 29, 2021 at 05:54:41PM +0100, Matthew Auld wrote:
> On Tue, 26 Oct 2021 at 23:51, Imre Deak <imre.deak at intel.com> wrote:
> >
> > During remapping CCS FBs the CCS AUX surface mapped size and offset->x,y
> > coordinate calculations assumed a tiled layout. This works as long as
> > the CCS surface height is aligned to 64 lines (ensuring a 4k bytes CCS
> > surface tile layout).  However this alignment is not required by the HW
> > (and the driver doesn't enforces it either).
> >
> > Add the remapping logic required to remap the pages of CCS surfaces
> > without the above alignment, assuming the natural linear layout of the
> > CCS surface (vs. tiled main surface layout).
> >
> > Cc: Juha-Pekka Heikkila <juhapekka.heikkila at gmail.com>
> > Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > Fixes: 3d1adc3d64cf ("drm/i915/adlp: Add support for remapping CCS FBs")
> > Signed-off-by: Imre Deak <imre.deak at intel.com>
> 
> <snip>
> 
> > diff --git a/drivers/gpu/drm/i915/i915_vma_types.h b/drivers/gpu/drm/i915/i915_vma_types.h
> > index 80e93bf00f2e5..4ee6e54799f48 100644
> > --- a/drivers/gpu/drm/i915/i915_vma_types.h
> > +++ b/drivers/gpu/drm/i915/i915_vma_types.h
> > @@ -97,11 +97,20 @@ enum i915_cache_level;
> >
> >  struct intel_remapped_plane_info {
> >         /* in gtt pages */
> > -       u32 offset;
> > -       u16 width;
> > -       u16 height;
> > -       u16 src_stride;
> > -       u16 dst_stride;
> > +       u32 offset:31;
> > +       u32 linear:1;
> > +       union {
> > +               /* in gtt pages for !linear */
> > +               struct {
> > +                       u16 width;
> > +                       u16 height;
> > +                       u16 src_stride;
> > +                       u16 dst_stride;
> > +               };
> > +
> > +               /* in gtt pages for linear */
> > +               u32 size;
> > +       };
> 
> Looks OK to me. Only concern is whether packed bitfields might give
> different results, depending on the compiler or something.

Granted, the C99 spec says:
"An implementation may allocate any addressable storage unit large
enough to hold a bit-field."

and

"The order of allocation of bit-fields within a unit (high-order to
low-order or low-order to high-order) is implementation-defined."

GCC and Clang at least behave the same (and most logical) way. The
packed attribute is also only a compiler extension, so I think we need
to rely anyway on what main stream compilers do.

The kernel has a few instances of packed bitfields used for some HW
interface and we also use this in IGT for GPU commands.

I could instead use macros to unpack the fields manually, but we could
do that only if bitfields prove to be a real concern (that BUILD_BUG()
in assert_i915_gem_gtt_types() would also indicate).

> If you look at the craziness in assert_i915_gem_gtt_types for example,
> it's very particular about the size/layout.

Yes, looks to be an optimization based on comments in
i915_vma_compare(), but I think the checks there ensure that the struct
layout is as expected.

> >  } __packed;
> >
> >  struct intel_remapped_info {
> > --
> > 2.27.0
> >