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

[Konduru, Chandra]

> -----Original Message-----
> From: Roper, Matthew D
> Sent: Thursday, April 09, 2015 2:51 PM
> To: Konduru, Chandra
> Cc: intel-gfx at 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 at 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);

> 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