Re: [Intel-gfx] [PATCH] drm/i915: Handle untiled planes when computing their offsets

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Thu, Feb 21, 2013 at 07:05:51PM +0000, Chris Wilson wrote:
> We trim the fb to fit the CRTC by computing the offset of that CRTC to
> its nearest tile_row origin. This allows us to use framebuffers that are
> larger than the CRTC limits without additional work.
> 
> However, we failed to compute the offset for a linear framebuffer
> correctly as we treated its x-advance in whole tiles (instead of the
> linear increment expected), leaving the CRTC misaligned with its
> contents.
> 
> Fixes regression from commit c2c75131244507c93f812862fdbd4f3a37139401
> Author: Daniel Vetter <daniel.vetter@ffwll.ch>
> Date:   Thu Jul 5 12:17:30 2012 +0200
> 
>     drm/i915: adjust framebuffer base address on gen4+
> 
> v2: Adjust relative x-coordinate after linear alignment (vsyrjala)
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=61152
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: stable@vger.kernel.org
> ---
>  drivers/gpu/drm/i915/intel_display.c |   33 +++++++++++++++++++++++----------
>  drivers/gpu/drm/i915/intel_drv.h     |    3 ++-
>  drivers/gpu/drm/i915/intel_sprite.c  |    6 ++++--
>  3 files changed, 29 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index a3ca9a8..c32c0dc 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -1906,18 +1906,29 @@ void intel_unpin_fb_obj(struct drm_i915_gem_object *obj)
>  
>  /* Computes the linear offset to the base tile and adjusts x, y. bytes per pixel
>   * is assumed to be a power-of-two. */
> -unsigned long intel_gen4_compute_offset_xtiled(int *x, int *y,
> -					       unsigned int bpp,
> +unsigned long intel_gen4_compute_planar_offset(int *x, int *y,
> +					       unsigned int tiling_mode,
> +					       unsigned int cpp,
>  					       unsigned int pitch)
>  {
> -	int tile_rows, tiles;
> +	if (tiling_mode != I915_TILING_NONE) {
> +		int tile_rows, tiles;
>  
> -	tile_rows = *y / 8;
> -	*y %= 8;
> -	tiles = *x / (512/bpp);
> -	*x %= 512/bpp;
> +		tile_rows = *y / 8;
> +		*y %= 8;
>  
> -	return tile_rows * pitch * 8 + tiles * 4096;
> +		tiles = *x / (512/cpp);
> +		*x %= 512/cpp;
> +
> +		return tile_rows * pitch * 8 + tiles * 4096;
> +	} else {
> +		int offset;

nit: unsigned

> +
> +		offset = *y * pitch + *x * cpp;
> +		*y = 0;
> +		*x = (offset & 4095) / cpp;
> +		return offset & -4096;

That should do it.

There is one small concern though. HSW BSpec tells us that the
offset + size mustn't exceed the maximum allowed sprite size.
That's a bit weird, but perhaps it just means that whaever
internal register that tracks the current x/y source positions
is the same size as the sprite size register.

Hmm, actually it seems I'm complaining about a non-issue. The
max horizontal size seems to be 13 bits, which means 8k, and the
max x offset from this functions is 2k. Given that displays are
probably limited to 4k, that would still leave us 2k to spare.
So I suppose we can just ignore this.

I do have one other small bikeshed though. Why does the function
name have planar in it? Maybe call it intel_gen4_compute_page_offset()
or something?

-- 
Ville Syrjälä
Intel OTC