Re: [PATCH 13/14] drm/i915: skylake primary plane scaling using shared scalers

From: Daniel Vetter <daniel@ffwll.ch>
To: "Konduru, Chandra" <chandra.konduru@intel.com>
Cc: "Vetter, Daniel" <daniel.vetter@intel.com>,
	"intel-gfx@lists.freedesktop.org"
	<intel-gfx@lists.freedesktop.org>,
	"Conselvan De Oliveira,
	Ander" <ander.conselvan.de.oliveira@intel.com>
Subject: Re: [PATCH 13/14] drm/i915: skylake primary plane scaling using shared scalers
Date: Mon, 4 May 2015 10:16:17 +0200	[thread overview]
Message-ID: <20150504081617.GL30184@phenom.ffwll.local> (raw)
In-Reply-To: <76A9B330A4D78C4D99CB292C4CC06C0E36FB647C@fmsmsx101.amr.corp.intel.com>

On Mon, Apr 27, 2015 at 05:13:49AM +0000, Konduru, Chandra wrote:
> 
> 
> > -----Original Message-----
> > From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel Vetter
> > Sent: Thursday, April 23, 2015 1:20 PM
> > To: Konduru, Chandra
> > Cc: intel-gfx@lists.freedesktop.org; Vetter, Daniel; Conselvan De Oliveira, Ander
> > Subject: Re: [Intel-gfx] [PATCH 13/14] drm/i915: skylake primary plane scaling
> > using shared scalers
> > 
> > On Wed, Apr 15, 2015 at 03:14:38PM -0700, Chandra Konduru wrote:
> > > This patch enables skylake primary plane scaling using shared
> > > scalers atomic desgin.
> > >
> > > v2:
> > > -use single copy of scaler limits (Matt)
> > >
> > > v3:
> > > -move detach_scalers to crtc commit path (Matt)
> > > -use values in plane_state->src as regular integers (me)
> > >
> > > v4:
> > > -changes to align with updated scaler structures (Matt, me)
> > > -keep plane src rect in 16.16 format (Matt, Daniel)
> > >
> > > v5:
> > > -Rebased on top of 90/270 rotation changes (me)
> > > -Fixed an issue introduced by 90/270 changes where plane programming
> > >  is using drm_plane->state rect instead of intel_plane->state rect.
> > >  This change also required for scaling to work properly. (me)
> > > -With 90/270, updated limits to cover both portrait and landscape usages (me)
> > > -Refactored skylake_update_primary_plane to reduce its size (Daniel)
> > >  Added helper functions for refactoring are comprehended enough to be
> > >  used for skylake_update_plane (for sprite) too. One stop towards
> > >  having single function for all planes.
> > >
> > > Signed-off-by: Chandra Konduru <chandra.konduru@intel.com>
> > > Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
> > > Testcase: kms_plane_scaling
> > 
> > I wanted to pull this in but then spotted an issue. Since this needs one
> > more round can you perhaps use the older version as a baseline again
> > (without the refactoring)? Since skl planes aren't converted to universal
> > planes yet it might be better to wait with refactoring until that's done
> > actually. Two more comments below.
> Hi Daniel,
> Sorry for delay due to osts/f2f travel.
> Per last discussion at osts, you were ok to merge the changes made to
> skl update_plane functions to reduce its size. To undo these
> changes I have to redo all testing and also have to redo changes
> to upcoming nv12 changes.
> skl planes are already universal planes, Matt can comment more
> on this. But irrespective of whether skl planes universal or not
> with below changes, function sizes are more manageable.

Yeah I looked at splitting up the refactoring, but because of the
divergent history of the primary and sprite planes (which converged now in
the hw for skl) it was messier than your patches here. Agreed that this is
the exception which confirms the rule ;-)
> 
> Regarding the FIXME and lock issue, will send out updated 
> patch shortly. 

Thanks, both applied to dinq.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx