[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 3:23 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 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 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);
> 
> 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.

OK, then I will send #6, #14 updated (i.e., moving hunk from #14 to #6) and
#9 with updated commit message to address scaler quality.
 
> 
> 
> 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