On Fri, Apr 23, 2021 at 08:04:35PM +0300, Ville Syrjälä wrote:
> On Wed, Apr 21, 2021 at 08:32:20PM +0300, Imre Deak wrote:
> > We can handle the surface alignment of CCS and UV color planes for all
> > modifiers at one place, so do this. An AUX color plane can be a CCS or a
> > UV plane, use only the more specific query functions and remove
> > is_aux_plane() becoming redundant.
> >
> > While at it add a TODO for linear UV color plane alignments. The spec
> > requires this to be stride-in-bytes * 64 on all platforms, whereas the
> > driver uses an alignment of 4k for gen<12 and 256k for gen>=12 for
> > linear UV planes.
> >
> > v2:
> > - Restore previous alignment for linear UV surfaces.
> >
> > Signed-off-by: Imre Deak <imre.deak@xxxxxxxxx>
> > ---
> > drivers/gpu/drm/i915/display/intel_display.c | 27 +++++++++++++-------
> > drivers/gpu/drm/i915/display/intel_fb.c | 8 ------
> > drivers/gpu/drm/i915/display/intel_fb.h | 1 -
> > 3 files changed, 18 insertions(+), 18 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > index a10e26380ef3d..e246e5cf75866 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -973,10 +973,26 @@ unsigned int intel_surf_alignment(const struct drm_framebuffer *fb,
> > struct drm_i915_private *dev_priv = to_i915(fb->dev);
> >
> > /* AUX_DIST needs only 4K alignment */
> > - if ((DISPLAY_VER(dev_priv) < 12 && is_aux_plane(fb, color_plane)) ||
> > - is_ccs_plane(fb, color_plane))
> > + if (is_ccs_plane(fb, color_plane))
> > return 4096;
> >
> > + if (is_semiplanar_uv_plane(fb, color_plane)) {
> > + /*
> > + * TODO: cross-check wrt. the bspec stride in bytes * 64 bytes
> > + * alignment for linear UV planes on all platforms.
> > + */
>
> I think it's just saying that UV should always start at an integer
> multiple of Y stride, whether we're dealing with linear or tiled.
This is what I see for SKL (bspec 18566):
"""
Linear:
The start of the UV surface programmed in the PLANE_AUX_DIST register
should be aligned to Stride in bytes * 64.
"""
The ICL page makes the above clearer that the above stride is the Y
plane stride, but I think it's always the same as the linear UV plane
stride.
Yes, I suppose the intention could have been to align to a multiple of
the stride, and maybe also ensure a page alignment.
> Dunno if that's true or not. I suppose there could be some
> tlb/prefetch related reasons for it.
Yea, not sure either. Simply following the above formula would result in
max_stride*64 = 128k * 64 = 8MB alignment requirement, which hopefully
isn't really needed.
> I think the same tile row/stride alignment requirements are specified
> for all gen9+ platforms actually. So if it's supposedly really needed
> then I guess we should do it on all platforms. And if it's not actually
> needed we shoud just nuke it all and be happy with 4k alignment.
I see the same UV alignment requirement on all platforms, but yes my
guess and hope is that only 4k is needed. I will do more tests to see if
misaligning the UV plane (only ensuring a 4k alignment) causes a problem
anywhere and ask for a clarification in bspec if not.
> What are the chances we can even find a suitbly aligned page boundary?
> Not sure.
The linear stride must be a multiple of 64, so stride*64 would always
result in a 4k alignment. The 64 multiplier in the formula could be
reduced in some cases (for instance when stride%4k==0), but in the worst
cases only *64 could provide an offset aligned both to 4k and to the
multiple of stride.
> Oh and there's some oddball mention of the UV start having to be a
> multiple of four lines. Is it talking about AUX_DIST of AUX_OFFSET.y?
> No idea. What lines? Maybe Y lines? Not sure.
Haven't noticed that. In my reading AUX_DIST has to be always page
aligned, but in case of linear,X-,Y-tiled formats the UV-start can
follow the Y surface end "immediately on the next line", so Y-end and
UV-start can be on the same page. I suppose in this layout there is
still the restriction that AUX_OFFSET.y should be a multiple of 4 (so
"immediately" in bpsec is not precise). Will test this out as well.
Another oddity I noticed now is the Yf format where the FB Y plane could
be allocated (with a good reason) with a different stride than the UV
plane? If so, bpsec says the driver should ensure that the Y and UV
scanout strides match (padding the allocated Y stride tile rows if
needed).
> > + if (DISPLAY_VER(dev_priv) >= 12) {
> > + if (fb->modifier == DRM_FORMAT_MOD_LINEAR)
> > + return intel_linear_alignment(dev_priv);
> > +
> > + return intel_tile_row_size(fb, color_plane);
> > + }
> > +
> > + return 4096;
> > + }
> > +
> > + drm_WARN_ON(&dev_priv->drm, color_plane != 0);
> > +
> > switch (fb->modifier) {
> > case DRM_FORMAT_MOD_LINEAR:
> > return intel_linear_alignment(dev_priv);
> > @@ -985,19 +1001,12 @@ unsigned int intel_surf_alignment(const struct drm_framebuffer *fb,
> > return 256 * 1024;
> > return 0;
> > case I915_FORMAT_MOD_Y_TILED_GEN12_MC_CCS:
> > - if (is_semiplanar_uv_plane(fb, color_plane))
> > - return intel_tile_row_size(fb, color_plane);
> > - fallthrough;
> > case I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS:
> > case I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC:
> > return 16 * 1024;
> > case I915_FORMAT_MOD_Y_TILED_CCS:
> > case I915_FORMAT_MOD_Yf_TILED_CCS:
> > case I915_FORMAT_MOD_Y_TILED:
> > - if (DISPLAY_VER(dev_priv) >= 12 &&
> > - is_semiplanar_uv_plane(fb, color_plane))
> > - return intel_tile_row_size(fb, color_plane);
> > - fallthrough;
> > case I915_FORMAT_MOD_Yf_TILED:
> > return 1 * 1024 * 1024;
>
> As for these IIRC TGL+ should not need any extra alignment anymore.
> But that's material for a separate patch.
Ok, can check this later.
> Anyways patch seems ok.
> Reviewed-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
Thanks.
> > default:
> > diff --git a/drivers/gpu/drm/i915/display/intel_fb.c b/drivers/gpu/drm/i915/display/intel_fb.c
> > index 0ec9ad7220a14..c8aaca3e79e97 100644
> > --- a/drivers/gpu/drm/i915/display/intel_fb.c
> > +++ b/drivers/gpu/drm/i915/display/intel_fb.c
> > @@ -30,14 +30,6 @@ bool is_gen12_ccs_cc_plane(const struct drm_framebuffer *fb, int plane)
> > plane == 2;
> > }
> >
> > -bool is_aux_plane(const struct drm_framebuffer *fb, int plane)
> > -{
> > - if (is_ccs_modifier(fb->modifier))
> > - return is_ccs_plane(fb, plane);
> > -
> > - return plane == 1;
> > -}
> > -
> > bool is_semiplanar_uv_plane(const struct drm_framebuffer *fb, int color_plane)
> > {
> > return intel_format_info_is_yuv_semiplanar(fb->format, fb->modifier) &&
> > diff --git a/drivers/gpu/drm/i915/display/intel_fb.h b/drivers/gpu/drm/i915/display/intel_fb.h
> > index 6acf792a8c44a..13244ec1ad214 100644
> > --- a/drivers/gpu/drm/i915/display/intel_fb.h
> > +++ b/drivers/gpu/drm/i915/display/intel_fb.h
> > @@ -19,7 +19,6 @@ struct intel_plane_state;
> > bool is_ccs_plane(const struct drm_framebuffer *fb, int plane);
> > bool is_gen12_ccs_plane(const struct drm_framebuffer *fb, int plane);
> > bool is_gen12_ccs_cc_plane(const struct drm_framebuffer *fb, int plane);
> > -bool is_aux_plane(const struct drm_framebuffer *fb, int plane);
> > bool is_semiplanar_uv_plane(const struct drm_framebuffer *fb, int color_plane);
> >
> > bool is_surface_linear(const struct drm_framebuffer *fb, int color_plane);
> > --
> > 2.27.0
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Ville Syrjälä
> Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
