> -----Original Message----- > From: Roper, Matthew D > Sent: Tuesday, April 07, 2015 11:46 AM > To: Konduru, Chandra > Cc: Daniel Vetter; Vetter, Daniel; intel-gfx@xxxxxxxxxxxxxxxxxxxxx; Conselvan De > Oliveira, Ander > Subject: Re: [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@xxxxxxxx] On Behalf Of > > > Daniel Vetter > > > Sent: Tuesday, April 07, 2015 1:44 AM > > > To: Roper, Matthew D > > > Cc: Konduru, Chandra; Vetter, Daniel; > > > intel-gfx@xxxxxxxxxxxxxxxxxxxxx; Conselvan De Oliveira, Ander > > > Subject: Re: [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@xxxxxxxxx> > > > > > > > > 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@xxxxxxxxxxxxxxx> > 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 _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
