| Message ID | 20191227235147.32366-1-imre.deak@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/3] drm/i915: Add support for non-power-of-2 FB plane alignment | expand |
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 <chris@chris-wilson.co.uk> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Signed-off-by: Imre Deak <imre.deak@intel.com> > --- > 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(&x, &y, plane_state, 0); > offset = intel_plane_compute_aligned_offset(&x, &y, 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@chris-wilson.co.uk> -Chris
On Mon, Dec 30, 2019 at 10:48:31AM +0000, 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 <chris@chris-wilson.co.uk> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Signed-off-by: Imre Deak <imre.deak@intel.com> > > --- > > 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(&x, &y, plane_state, 0); > > offset = intel_plane_compute_aligned_offset(&x, &y, 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@chris-wilson.co.uk> > -Chris
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(&x, &y, plane_state, 0); offset = intel_plane_compute_aligned_offset(&x, &y, 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
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 <chris@chris-wilson.co.uk> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Signed-off-by: Imre Deak <imre.deak@intel.com> --- drivers/gpu/drm/i915/display/intel_display.c | 22 +++++++++++++------- 1 file changed, 14 insertions(+), 8 deletions(-)