Re: [PATCH] drm/i915: Fix SKL+ 90/270 degree rotated plane coordinate computation

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Tue, Oct 25, 2016 at 05:02:15PM -0200, Paulo Zanoni wrote:
> Em Seg, 2016-10-24 às 19:13 +0300, ville.syrjala@linux.intel.com
> escreveu:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Pass the framebuffer size in .16 fixed point coordinates to
> > drm_rect_rotate() since that's what the source coordinates are as
> > well
> > at this stage. We used to do this part of the computation in integer
> > coordinates, but that got changed when moving the computation to
> > happen in the check phase of the operation. Unfortunately I forgot
> > to shift up the fb width and height appropriately.
> > 
> > With the bogus size we ended up with some negative fb offset, which
> > when
> > added to the vma offset caused out scanout to start at an offset
> > earlier
> > than we inteded. Eg. when testing on my SKL I saw a row of incorrect
> > tiles at the top of my screen.
> 
> I already mentioned this while reviewing another patch from another
> author, but let's throw the idea again: how about adding a specific
> 16.16 type in order to prevent these silent failures when mixing them
> with integers?
> 
> struct (or union) int_fixed_16_16 {
> 	uint32_t number;
> }
> 
> And them some nice macros for explicit casting to/from int.
> 
> I see include/drm/fixed.h even has a 20_12 type...

I just get hopelessly confused when fixed point math is hidden in
some library. But who knows, perhaps someone might be able to write
one that my brain can handle. So far I've not seen one though.

> 
> I could even volunteer to do this if there's some positive feedback
> upfront, but feel free to do this too in case you want.
> 
> We're humans and we're going to make the same "mix normal integers with
> 16.16 integers" mistake again and again and again, while the compiler
> could really help us if the types were not plain integers.
> 
> Thoughts?
> 
> > 
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > Cc: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com>
> > Cc: drm-intel-fixes@lists.freedesktop.org
> > Fixes: b63a16f6cd89 ("drm/i915: Compute display surface offset in the
> > plane check hook for SKL+")
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > b/drivers/gpu/drm/i915/intel_display.c
> > index 5a036999487b..c783f884f85d 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -2985,7 +2985,8 @@ int skl_check_plane_surface(struct
> > intel_plane_state *plane_state)
> >  	/* Rotate src coordinates to match rotated GTT view */
> >  	if (drm_rotation_90_or_270(rotation))
> >  		drm_rect_rotate(&plane_state->base.src,
> > -				fb->width, fb->height,
> > DRM_ROTATE_270);
> > +				fb->width << 16, fb->height << 16,
> > +				DRM_ROTATE_270);
> >  
> >  	/*
> >  	 * Handle the AUX surface first since

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx