Re: [PATCH 00/22] drm/i915: Handle fb->offsets[] and rewrite fb rotation handling to be more generic

From: Daniel Vetter <daniel@ffwll.ch>
To: ville.syrjala@linux.intel.com
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 00/22] drm/i915: Handle fb->offsets[] and rewrite fb rotation handling to be more generic
Date: Wed, 21 Oct 2015 17:15:53 +0200	[thread overview]
Message-ID: <20151021151553.GD16848@phenom.ffwll.local> (raw)
In-Reply-To: <20151021120900.GN13786@phenom.ffwll.local>

On Wed, Oct 21, 2015 at 02:09:00PM +0200, Daniel Vetter wrote:
> On Wed, Oct 14, 2015 at 06:59:38PM +0200, Daniel Vetter wrote:
> > On Wed, Oct 14, 2015 at 07:28:52PM +0300, ville.syrjala@linux.intel.com wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > So while reviewing the NV12 stuff it became clear to me no one
> > > had really given fb->offsets[] handling any serious thought.
> > > So this patch series aims to fix that. We now treat fb->offsets[]
> > > as a linear offset always. One clear benefit over treating it as
> > > a linear offset as opposed to a raw byte offset is that we don't
> > > have to think about the layout of bytes within the tile at all.
> > > 
> > > The series also generalizes the page rotation to be format agnostic,
> > > the caller just specifies the desired geometry in pages for each
> > > plane, and the rotation code builds up the sg. The intel_rotation_info
> > > then just contains the minimal amount of information needed to
> > > do the page rotation.
> > > 
> > > SKL+ also gets changed to use the compute_page_offset stuff so that
> > > the plane SURF register will contain the closes (properly aligned)
> > > page boundary, and the x/y offsets deal with whatever is left over.
> > > The plane code for the other platforms also gets simpler in the end
> > > I think. Also the 90/270 rotation handling becomes rather trivial
> > > for the plane code.
> > > 
> > > I should still write some decent tests to exercise fb->offsets[].
> > > 
> > > Series available here:
> > > git://github.com/vsyrjala/linux.git fb_offsets
> > 
> > As mentioned on irc patches 13&14 need to me unified with my 3 patch
> > series, but otherwise lgtm overall.
> 
> Ok done a review pass and I think I unconfused myself about the bytes vs.
> pixels fun. Imo that really needs to be a lot more explicit. My
> suggestion:
> 
> - tile_width, tile_height, pixel_stride: always pixels
> - tile_pitch, pitch: always bytes
> 
> And instead of silently changing units just use new variables. gcc will
> clean it up. I'm totally ok with doing that as a follow-up.
> 
> I didn't review the 2nd last patch in detail since -ETOOBIG.

Another thing that crossed my mind: do we have testcase for rotated 16bpp
framebuffers? All the confusion between pixels and bytes and my confusion
with Yf tiling suggests that might be useful.
-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