Re: [PATCH 1/5] drm/i915: Fix scaling check for 90/270 degree plane rotation

From: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
To: Chris Wilson <chris@chris-wilson.co.uk>,
	Tvrtko Ursulin <tvrtko.ursulin@intel.com>,
	intel-gfx@lists.freedesktop.org,
	drm-intel-fixes@lists.freedesktop.org
Subject: Re: [PATCH 1/5] drm/i915: Fix scaling check for 90/270 degree plane rotation
Date: Tue, 8 Nov 2016 12:58:46 +0200	[thread overview]
Message-ID: <20161108105846.GC4617@intel.com> (raw)
In-Reply-To: <20161107212858.GT30925@nuc-i3427.alporthouse.com>

On Mon, Nov 07, 2016 at 09:28:58PM +0000, Chris Wilson wrote:
> On Mon, Nov 07, 2016 at 10:20:53PM +0200, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Starting from commit b63a16f6cd89 ("drm/i915: Compute display surface
> > offset in the plane check hook for SKL+") we've already rotated the src
> > coordinates by 270 degrees by the time we check if a scaler is needed
> > or not, so we must not account for the rotation a second time.
> > Previously we did these steps in the opposite order and hence the
> > scaler check had to deal with rotation itself. The double rotation
> > handling causes us to enable a scaler pretty much every time 90/270
> > degree plane rotation is requested, leading to fuzzier fonts and whatnot.
> > 
> > Cc: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > Cc: drm-intel-fixes@lists.freedesktop.org
> > Reported-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > Fixes: b63a16f6cd89 ("drm/i915: Compute display surface offset in the plane check hook for SKL+")
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> I believe
> 
> 	Tested-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Thanks, forgot to amend it.

> 
> skl_plane_downscale_amount() ?
> skl_plane_relative_data_rate() ?
> skl_ddb_min_alloc()
> skl_compute_plane_wm()

Oh dear. I may have broken the world. Looks like a full blown review
of all uses of the src rect is required.

> What's the best way of keeping the reader aware of whether or not the
> coordinate transform has been applied? Similar to how to let the user
> know the rect is in fixed_16_16 or int32.

That's a good question. I guess for the fixed point stuff we could
define a new type, but then we can't use the same functions to massage
it, which doesn't sound all that nice. Hmm. Maybe we need to add some
metadata to the struct?

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