> -----Original Message-----
> From: Roper, Matthew D
> Sent: Thursday, April 09, 2015 2:51 PM
> To: Konduru, Chandra
> Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx; 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@xxxxxxxxx>
>
> 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
