[Intel-gfx] [PATCH 06/20] drm/i915: Convert primary plane 16.16 values to regular ints

[Konduru, Chandra]

> -----Original Message-----
> From: Daniel Vetter [mailto:daniel.vetter at ffwll.ch] On Behalf Of Daniel Vetter
> Sent: Tuesday, April 07, 2015 1:44 AM
> To: Roper, Matthew D
> Cc: Konduru, Chandra; Vetter, Daniel; intel-gfx at lists.freedesktop.org;
> Conselvan De Oliveira, Ander
> Subject: Re: [Intel-gfx] [PATCH 06/20] drm/i915: Convert primary plane 16.16
> values to regular ints
> 
> On Thu, Apr 02, 2015 at 04:03:22PM -0700, Matt Roper wrote:
> > On Wed, Apr 01, 2015 at 07:59:35PM -0700, Chandra Konduru wrote:
> > > This patch converts intel_plane_state->src rect from 16.16 values
> > > into regular ints.
> > >
> > > This approach aligns with sprite_plane_state->src rects which are
> > > already in regular ints.
> > >
> > > Signed-off-by: Chandra Konduru <chandra.konduru at intel.com>
> >
> > You're not touching cursor state here, so I guess it stays in 16.16
> > form always?  Does it need to be updated to behave the same way?
> >
> > Looking at our sprite code today, it treats intel_state->src as 16.16
> > for most of the check function, then re-writes it as integer pixels
> > near the end, which I guess matches the type of change you're doing
> > here.  I still find this pretty confusing that our structure is
> > sometimes interpreted in one way and other times interpreted a different way.
> >
> > Personally I think it would be less error-prone if we just treated src
> > as 16.16 always, but if you to keep the current logic which changes
> > the meaning at the end of the check() stage, can you add some comments
> > to struct intel_plane_state clarifying that?
> 
> Rewriting intel_plane_state won't work since on duplicate_state we'd need to
> undo that again. That's a bit too brittle imo. I'd go with Matt's suggestion to just
> use 16.16 everywhere.
> -Daniel

Recently in upstream, sprite plane src rect changed to regular int at end of check
before entering commit but left primary src rect as 16.16.

This is causing issue for having any common helper function to handle sprites
and primary. So my patch is aligning both primary and sprite's src rect as regular int
after checks are done. 

Earlier Matt commented that it is upto i915's implementation how to keep
its internal src rect values in its state which is his response to my earlier 
change to fix a bug when passing src rect keeping them in 16.16 format.  
I am fine whichever format you want. Can you or Matt make it clear which
format to keep them?

> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch