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

[Konduru, Chandra]

> -----Original Message-----
> From: Roper, Matthew D
> Sent: Tuesday, April 07, 2015 11:46 AM
> To: Konduru, Chandra
> Cc: Daniel Vetter; 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 Tue, Apr 07, 2015 at 11:29:06AM -0700, Konduru, Chandra wrote:
> >
> >
> > > -----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?
> 
> The internal src/dest rect are derived state that the driver tracks for its own
> purposes, so yeah, it's up to us how we decide to store it.  The confusing (and
> error-prone) part is that our sprite check code treats it as 16.16 whereas our
> commit code treats it as integer; to make matters worse, we aren't even
> consistent with this scheme across the various plane types (primary, sprite,
> cursor).
> 
> Even though the state gets copied in duplicate_state(), we don't have any
> immediate problems with the existing sprite code today because it does wind up
> getting blown away and recomputed before we have a chance to mix up the
> meaning of the values.  Your patch here looks like it would work without
> problems today too for the same reason.  But simply the inconsistency of what
> the value means at various points in the process bothers me because it is likely
> to cause mistakes as people write code in the future.  Since the check phase is
> the more complex logic here, and since it depends on the 16.16 storage, it
> seems natural to me to just preserve that 16.16 format throughout and just shift
> as necessary in the commit step.
> 
> From what I can see, the mid-operation switch between 16.16 and integer
> format in the sprite code originated with commit
> 
>         commit 96d61a7f267ff355a401ca23a732810027d10ba2
>         Author: Gustavo Padovan <gustavo.padovan at collabora.co.uk>
>         Date:   Fri Sep 5 17:04:47 2014 -0300
> 
>             drm/i915: split intel_update_plane into check() and commit()
> 
> when the check and commit steps were first split apart.  The change isn't directly
> referenced in the commit message, so I think it just sort of snuck in under the
> radar.

Today platform_update_plane(x, y, src_w, src_h) and 
platform_update_primary_plane(x, y)
parameters are in regular integer format. 
keeping them as is.

Will bring src rect back into 16.16 format. And any consumer of src
will do >> 16 on src values as needed.

> 
> 
> Matt
> 
> >
> > > --
> > > Daniel Vetter
> > > Software Engineer, Intel Corporation http://blog.ffwll.ch
> 
> --
> Matt Roper
> Graphics Software Engineer
> IoTG Platform Enabling & Development
> Intel Corporation
> (916) 356-2795