Re: [Intel-gfx] [PATCH 1/3] drm/i915: Add support for non-power-of-2 FB plane alignment
On Mon, Dec 30, 2019 at 10:48:31AM +, Chris Wilson wrote:
> Quoting Imre Deak (2019-12-27 23:51:45)
> > At least one framebuffer plane on TGL - the UV plane of YUV semiplanar
> > FBs - requires a non-power-of-2 alignment, so add support for this. This
> > new alignment restriction applies only to an offset within an FB, so the
> > GEM buffer itself containing the FB must still be power-of-2 aligned.
>
> It's worth talking about virtual memory alignment (in the GGTT) here and
> not the physical alignment of the backing store. The buffer itself plays
> no part here.
Yes, this new restriction is about the GGTT mapping and display
specific. In fact other engines have other restrictions when
reading/writing the same YUV surfaces - for instance via a PPGTT map.
And yes, the page physical addresses can be anything.
Will improve the commit log.
> > Add a check for this (in practice plane 0, since the plane 0 offset must
> > be 0).
> >
> > Cc: Chris Wilson
> > Cc: Ville Syrjälä
> > Signed-off-by: Imre Deak
> > ---
> > drivers/gpu/drm/i915/display/intel_display.c | 22 +---
> > 1 file changed, 14 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> > b/drivers/gpu/drm/i915/display/intel_display.c
> > index 624ba9be7293..d8970198c77e 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -2194,6 +2194,7 @@ intel_pin_and_fence_fb_obj(struct drm_framebuffer *fb,
> > return ERR_PTR(-EINVAL);
> >
> > alignment = intel_surf_alignment(fb, 0);
> > + WARN_ON(!is_power_of_2(alignment));
>
> Handle the error, if you are going to the trouble to warn, add the
> return as well.
Ok.
> >
> > /* Note that the w/a also requires 64 PTE of padding following the
> > * bo. We currently fill all unused PTE with the shadow page and so
> > @@ -2432,9 +2433,6 @@ static u32 intel_compute_aligned_offset(struct
> > drm_i915_private *dev_priv,
> > unsigned int cpp = fb->format->cpp[color_plane];
> > u32 offset, offset_aligned;
> >
> > - if (alignment)
> > - alignment--;
> > -
> > if (!is_surface_linear(fb, color_plane)) {
> > unsigned int tile_size, tile_width, tile_height;
> > unsigned int tile_rows, tiles, pitch_tiles;
> > @@ -2456,17 +2454,24 @@ static u32 intel_compute_aligned_offset(struct
> > drm_i915_private *dev_priv,
> > *x %= tile_width;
> >
> > offset = (tile_rows * pitch_tiles + tiles) * tile_size;
> > - offset_aligned = offset & ~alignment;
> > +
> > + offset_aligned = offset;
> > + if (alignment)
> > + offset_aligned = rounddown(offset_aligned,
> > alignment);
> >
> > intel_adjust_tile_offset(x, y, tile_width, tile_height,
> > tile_size, pitch_tiles,
> > offset, offset_aligned);
> > } else {
> > offset = *y * pitch + *x * cpp;
> > - offset_aligned = offset & ~alignment;
> > -
> > - *y = (offset & alignment) / pitch;
> > - *x = ((offset & alignment) - *y * pitch) / cpp;
> > + offset_aligned = offset;
> > + if (alignment) {
> > + offset_aligned = rounddown(offset_aligned,
> > alignment);
> > + *y = (offset % alignment) / pitch;
> > + *x = ((offset % alignment) - *y * pitch) / cpp;
> > + } else {
> > + *y = *x = 0;
> > + }
> > }
> >
> > return offset_aligned;
> > @@ -3738,6 +3743,7 @@ static int skl_check_main_surface(struct
> > intel_plane_state *plane_state)
> > intel_add_fb_offsets(, , plane_state, 0);
> > offset = intel_plane_compute_aligned_offset(, , plane_state, 0);
> > alignment = intel_surf_alignment(fb, 0);
> > + WARN_ON(!is_power_of_2(alignment));
>
> The other two are expected to handle !is_pot...
Not sure what the alignment of the address we write to the main surface
address register should be, the spec doesn't say anything about that. I
assume now that not wrapping around the right edge of the detiler fence
we add is enough (which is also checked in intel_fill_fb_info()).
> I would strongly suggest handling the WARNs, or else you may as well bug
> out for the programming error.
Ok, will change those.
> Reviewed-by: Chris Wilson
> -Chris
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/3] drm/i915: Add support for non-power-of-2 FB plane alignment
Quoting Imre Deak (2019-12-27 23:51:45)
> At least one framebuffer plane on TGL - the UV plane of YUV semiplanar
> FBs - requires a non-power-of-2 alignment, so add support for this. This
> new alignment restriction applies only to an offset within an FB, so the
> GEM buffer itself containing the FB must still be power-of-2 aligned.
It's worth talking about virtual memory alignment (in the GGTT) here and
not the physical alignment of the backing store. The buffer itself plays
no part here.
> Add a check for this (in practice plane 0, since the plane 0 offset must
> be 0).
>
> Cc: Chris Wilson
> Cc: Ville Syrjälä
> Signed-off-by: Imre Deak
> ---
> drivers/gpu/drm/i915/display/intel_display.c | 22 +---
> 1 file changed, 14 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> b/drivers/gpu/drm/i915/display/intel_display.c
> index 624ba9be7293..d8970198c77e 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -2194,6 +2194,7 @@ intel_pin_and_fence_fb_obj(struct drm_framebuffer *fb,
> return ERR_PTR(-EINVAL);
>
> alignment = intel_surf_alignment(fb, 0);
> + WARN_ON(!is_power_of_2(alignment));
Handle the error, if you are going to the trouble to warn, add the
return as well.
>
> /* Note that the w/a also requires 64 PTE of padding following the
> * bo. We currently fill all unused PTE with the shadow page and so
> @@ -2432,9 +2433,6 @@ static u32 intel_compute_aligned_offset(struct
> drm_i915_private *dev_priv,
> unsigned int cpp = fb->format->cpp[color_plane];
> u32 offset, offset_aligned;
>
> - if (alignment)
> - alignment--;
> -
> if (!is_surface_linear(fb, color_plane)) {
> unsigned int tile_size, tile_width, tile_height;
> unsigned int tile_rows, tiles, pitch_tiles;
> @@ -2456,17 +2454,24 @@ static u32 intel_compute_aligned_offset(struct
> drm_i915_private *dev_priv,
> *x %= tile_width;
>
> offset = (tile_rows * pitch_tiles + tiles) * tile_size;
> - offset_aligned = offset & ~alignment;
> +
> + offset_aligned = offset;
> + if (alignment)
> + offset_aligned = rounddown(offset_aligned, alignment);
>
> intel_adjust_tile_offset(x, y, tile_width, tile_height,
> tile_size, pitch_tiles,
> offset, offset_aligned);
> } else {
> offset = *y * pitch + *x * cpp;
> - offset_aligned = offset & ~alignment;
> -
> - *y = (offset & alignment) / pitch;
> - *x = ((offset & alignment) - *y * pitch) / cpp;
> + offset_aligned = offset;
> + if (alignment) {
> + offset_aligned = rounddown(offset_aligned, alignment);
> + *y = (offset % alignment) / pitch;
> + *x = ((offset % alignment) - *y * pitch) / cpp;
> + } else {
> + *y = *x = 0;
> + }
> }
>
> return offset_aligned;
> @@ -3738,6 +3743,7 @@ static int skl_check_main_surface(struct
> intel_plane_state *plane_state)
> intel_add_fb_offsets(, , plane_state, 0);
> offset = intel_plane_compute_aligned_offset(, , plane_state, 0);
> alignment = intel_surf_alignment(fb, 0);
> + WARN_ON(!is_power_of_2(alignment));
The other two are expected to handle !is_pot...
I would strongly suggest handling the WARNs, or else you may as well bug
out for the programming error.
Reviewed-by: Chris Wilson
-Chris
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 1/3] drm/i915: Add support for non-power-of-2 FB plane alignment
At least one framebuffer plane on TGL - the UV plane of YUV semiplanar
FBs - requires a non-power-of-2 alignment, so add support for this. This
new alignment restriction applies only to an offset within an FB, so the
GEM buffer itself containing the FB must still be power-of-2 aligned.
Add a check for this (in practice plane 0, since the plane 0 offset must
be 0).
Cc: Chris Wilson
Cc: Ville Syrjälä
Signed-off-by: Imre Deak
---
drivers/gpu/drm/i915/display/intel_display.c | 22 +---
1 file changed, 14 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_display.c
b/drivers/gpu/drm/i915/display/intel_display.c
index 624ba9be7293..d8970198c77e 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -2194,6 +2194,7 @@ intel_pin_and_fence_fb_obj(struct drm_framebuffer *fb,
return ERR_PTR(-EINVAL);
alignment = intel_surf_alignment(fb, 0);
+ WARN_ON(!is_power_of_2(alignment));
/* Note that the w/a also requires 64 PTE of padding following the
* bo. We currently fill all unused PTE with the shadow page and so
@@ -2432,9 +2433,6 @@ static u32 intel_compute_aligned_offset(struct
drm_i915_private *dev_priv,
unsigned int cpp = fb->format->cpp[color_plane];
u32 offset, offset_aligned;
- if (alignment)
- alignment--;
-
if (!is_surface_linear(fb, color_plane)) {
unsigned int tile_size, tile_width, tile_height;
unsigned int tile_rows, tiles, pitch_tiles;
@@ -2456,17 +2454,24 @@ static u32 intel_compute_aligned_offset(struct
drm_i915_private *dev_priv,
*x %= tile_width;
offset = (tile_rows * pitch_tiles + tiles) * tile_size;
- offset_aligned = offset & ~alignment;
+
+ offset_aligned = offset;
+ if (alignment)
+ offset_aligned = rounddown(offset_aligned, alignment);
intel_adjust_tile_offset(x, y, tile_width, tile_height,
tile_size, pitch_tiles,
offset, offset_aligned);
} else {
offset = *y * pitch + *x * cpp;
- offset_aligned = offset & ~alignment;
-
- *y = (offset & alignment) / pitch;
- *x = ((offset & alignment) - *y * pitch) / cpp;
+ offset_aligned = offset;
+ if (alignment) {
+ offset_aligned = rounddown(offset_aligned, alignment);
+ *y = (offset % alignment) / pitch;
+ *x = ((offset % alignment) - *y * pitch) / cpp;
+ } else {
+ *y = *x = 0;
+ }
}
return offset_aligned;
@@ -3738,6 +3743,7 @@ static int skl_check_main_surface(struct
intel_plane_state *plane_state)
intel_add_fb_offsets(, , plane_state, 0);
offset = intel_plane_compute_aligned_offset(, , plane_state, 0);
alignment = intel_surf_alignment(fb, 0);
+ WARN_ON(!is_power_of_2(alignment));
/*
* AUX surface offset is specified as the distance from the
--
2.23.1
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
