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

Konduru, Chandra
2015-04-07T19:02:58Z

> -----Original Message-----
> From: Roper, Matthew D
> Sent: Tuesday, April 07, 2015 11:46 AM
> To: Konduru, Chandra
> Cc: Daniel Vetter; Vetter, Daniel; intel-gfx@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@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@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@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@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