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@xxxxxxxxx> 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@xxxxxxxxx>
> > Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > Fixes: 3d1adc3d64cf ("drm/i915/adlp: Add support for remapping CCS FBs")
> > Signed-off-by: Imre Deak <imre.deak@xxxxxxxxx>
>
> <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
> >
