Re: [PATCH 4/4] drm/i915: Setup clipped src/dest coordinates during FB reconstruction

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

On Thu, Nov 12, 2015 at 04:31:59PM -0800, Matt Roper wrote:
> Plane state objects contain two copies of src/dest coordinates:  the
> original (requested by userspace) coordinates in the base
> drm_plane_state object, and a second, clipped copy (i.e., what we
> actually want to program to the hardware) in intel_plane_state.  We've
> only been setting up the former set of values during boot time FB
> reconstruction, but we should really be initializing both.
> 
> Note that the code here probably still needs some more work.  We seem to
> be making two questionable assumptions:
>  * BIOS will program the primary plane to the entire display size
>    (potentially false on gen9+ platforms where the primary plane can be
>    windowed)

We don't actually assume that on SKL+. We read out the plane size
register and use that to set the initial fb dimensions. Although if
the BIOS uses a scaled plane, we will get the wrong answer since then
the plane size register will contain the dst size, not the src.

>  * The BIOS fb size actually matches the plane/display size.  It seems
>    that there's nothing stopping the BIOS from overallocating the FB
>    (potentially to do an extended mode FB that spans multiple displays),
>    so setting our src/dest coordinates to the FB size here may not
>    always be right.

I suppose we could have some kind of post-process step after the readout
to see if all the planes actually scan out from different parts of the
same fb. But that seems rather unlikely to me.

The other assumption we do is that the BIOS sets up GTT to 1:1
map the stolen memory. Not sure if some BIOSen would for instance
try to avoid the first page of stolen on gen8 (with EFI it should be
doable I suppose, with VGA probably not), or maybe sets up a 90/270
rotated mapping instead. I have one BYT tablet where the BIOS uses
180 degree rotation, so even 90/270 degree rotation is not totally
out of the question I think.

> 
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 9fa041c..3145c9d 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2601,6 +2601,8 @@ intel_find_initial_plane_obj(struct intel_crtc *intel_crtc,
>  	struct drm_i915_gem_object *obj;
>  	struct drm_plane *primary = intel_crtc->base.primary;
>  	struct drm_plane_state *plane_state = primary->state;
> +	struct intel_plane_state *intel_state =
> +		to_intel_plane_state(plane_state);
>  	struct drm_framebuffer *fb;
>  
>  	if (!plane_config->fb)
> @@ -2648,6 +2650,15 @@ valid_fb:
>  	plane_state->crtc_w = fb->width;
>  	plane_state->crtc_h = fb->height;
>  
> +	intel_state->src.x1 = plane_state->src_x;
> +	intel_state->src.y1 = plane_state->src_y;
> +	intel_state->src.x2 = plane_state->src_x + plane_state->src_w;
> +	intel_state->src.y2 = plane_state->src_y + plane_state->src_h;
> +	intel_state->dst.x1 = plane_state->crtc_x;
> +	intel_state->dst.y1 = plane_state->crtc_y;
> +	intel_state->dst.x2 = plane_state->crtc_x + plane_state->crtc_w;
> +	intel_state->dst.y2 = plane_state->crtc_y + plane_state->crtc_h;

Patch looks sane.
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> +
>  	obj = intel_fb_obj(fb);
>  	if (obj->tiling_mode != I915_TILING_NONE)
>  		dev_priv->preserve_bios_swizzle = true;
> -- 
> 2.1.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx