Re: [PATCH 06/14] drm/i915: Keep sprite plane src rect in 16.16 format

[Matt Roper <matthew.d.roper@intel.com>]

On Thu, Apr 09, 2015 at 03:08:55PM -0700, Konduru, Chandra wrote:
> 
> 
> > -----Original Message-----
> > From: Roper, Matthew D
> > Sent: Thursday, April 09, 2015 2:51 PM
> > To: Konduru, Chandra
> > Cc: intel-gfx@lists.freedesktop.org; Vetter, Daniel; Conselvan De Oliveira, Ander
> > Subject: Re: [PATCH 06/14] drm/i915: Keep sprite plane src rect in 16.16 format
> > 
> > On Tue, Apr 07, 2015 at 03:28:39PM -0700, Chandra Konduru wrote:
> > > This patch keeps intel_plane_state->src rect back into 16.16 format.
> > >
> > > v2:
> > > -sprite src rect to match primary format (Matt, Daniel)
> > >
> > > Signed-off-by: Chandra Konduru <chandra.konduru@intel.com>
> > 
> > This looks good, and matches what we had discussed, but don't you also need to
> > add the corresponding unshift in intel_commit_sprite_plane() when we actually
> > pull the values out and make use of them?  
> The unshift is in patch #14 which should have been in this patch.
> 
> From #14 relevant hunk is:
> @@ -1081,10 +1122,10 @@ intel_commit_sprite_plane(struct drm_plane *plane,
>             crtc_y = state->dst.y1;
>             crtc_w = drm_rect_width(&state->dst);
>             crtc_h = drm_rect_height(&state->dst);
> -           src_x = state->src.x1;
> -           src_y = state->src.y1;
> -           src_w = drm_rect_width(&state->src);
> -           src_h = drm_rect_height(&state->src);
> +           src_x = state->src.x1 >> 16;
> +           src_y = state->src.y1 >> 16;
> +           src_w = drm_rect_width(&state->src) >> 16;
> +           src_h = drm_rect_height(&state->src) >> 16;
>             intel_plane->update_plane(plane, crtc, fb,
>                           crtc_x, crtc_y, crtc_w, crtc_h,
>                           src_x, src_y, src_w, src_h);

Yep, you're right; I got the patch numbers mixed up while flipping back
and forth between patches, and then confused myself further by looking
at the wrong patch while writing my follow up reply.

As long as we pull this hunk from #14 back into this patch, that should
be the proper fix.  You can ignore my other comments below where I was
just confusing myself by looking at the wrong patch numbers.


Matt

> 
> > The goal is to keep the meaning of
> > the structure fields consistent at all times
> > (16.16 fixed pt), but once we pull the values out of the structure, we wind up
> > passing them to functions that doing use fixed point, so we do need to unshift at
> > that point.
> > 
> > It looks like in patch #13 you do switch the low-level
> > skl_update_plane() to make use of 16.16 input parameters.  However any
> > commit that we bisect through between #6 and #13 is going to wind up treating
> > 16.16 values as integer values, which I assume will blow up.
> Patch #13 doesn't touches skl_update_plane(). Perhaps you may be referring to #14.
> In #14, it does unshift as I mentioned above. I think I need to move above hunk
> to #6 to fix any potential issue due to bisect.
> 
> Let me know if you see any potential issue after moving the above hunk to #6.
> 
> > Also, you haven't touched any of the other platforms (ilk, vlv, ivb,
> > etc.) so they're all still going to have problems.
> Shifting and unshifting is happening in intel_check_plane and intel_commit_plane
> which is common for all platforms. I don't see what the problem you
> are referring to?
> > 
> > I think the easiest short-term solution is to do the unshifting in commit_plane
> > and leave the hardware-specific programming functions as-is.  
> This is what I'm doing now keeping platform_update_plane(parameters) use
> unshifted values (i.e., regular ints). I don't see any value to pass function parameters
> as 16.16 values. If there is a need arise we can change the semantics of parameters
> at a later time. 
> 
> > Longer term,
> > maybe it makes sense for a future patchset to change the actual register-
> > programming functions (foo_update_plane) so that they take a plane_state
> > directly and do their own unshifting?  In that case we'd need to update them to
> > do their own unshifting, but at least we wouldn't have to pull all the values out in
> > commit_plane, just to pass them to these functions.
> > 
> > 
> > Matt
> > 
> > > ---
> > >  drivers/gpu/drm/i915/intel_sprite.c |    8 ++++----
> > >  1 file changed, 4 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/intel_sprite.c
> > > b/drivers/gpu/drm/i915/intel_sprite.c
> > > index ac4aa68..c05fb36 100644
> > > --- a/drivers/gpu/drm/i915/intel_sprite.c
> > > +++ b/drivers/gpu/drm/i915/intel_sprite.c
> > > @@ -1006,10 +1006,10 @@ intel_check_sprite_plane(struct drm_plane
> > *plane,
> > >  	}
> > >
> > >  	if (state->visible) {
> > > -		src->x1 = src_x;
> > > -		src->x2 = src_x + src_w;
> > > -		src->y1 = src_y;
> > > -		src->y2 = src_y + src_h;
> > > +		src->x1 = src_x << 16;
> > > +		src->x2 = (src_x + src_w) << 16;
> > > +		src->y1 = src_y << 16;
> > > +		src->y2 = (src_y + src_h) << 16;
> > >  	}
> > >
> > >  	dst->x1 = crtc_x;
> > > --
> > > 1.7.9.5
> > >
> > 
> > --
> > Matt Roper
> > Graphics Software Engineer
> > IoTG Platform Enabling & Development
> > Intel Corporation
> > (916) 356-2795

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx