Re: [Intel-gfx] [PATCH 1/2] drm: Adding rotation to drm_plane_helper_check_update

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

On Thu, Jan 15, 2015 at 02:04:02AM +0000, Jindal, Sonika wrote:
> So, if we do the source and dst adjustments in userspace, the logic in intel_check_sprite_plane will not hold good there.

I'm not sure what adjustments you mean. Src says exactly which part of
the FB you want to present to the user, rotation doesn't change that
fact. Dst says exactly where on the screen you want the image to appear,
again rotation doesn't change that fact.

> That's how it gets right coordinates and w,h for sprite for 90 rotation.
> I thought drm_rect_rotate and other functions are there to handle such scenarios.

They are there mostly to make sure clipping happens correctly. They
aren't there to magically fix userspace bugs.

The only magicy part is the relaxed scaling and the rounding of
coordinates based on hardware limits, and that stuff only exists so
that it's easier to write userspace code without having to know
the limitations of the hardware.

> 
> -----Original Message-----
> From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] 
> Sent: Wednesday, January 14, 2015 11:38 PM
> To: Jindal, Sonika
> Cc: intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH 1/2] drm: Adding rotation to drm_plane_helper_check_update
> 
> On Wed, Jan 14, 2015 at 01:54:06PM +0000, Jindal, Sonika wrote:
> > I think I am confused here..
> > Since sprite plane handles this scaling and rotation in kernel, I thought it should be taken care in driver for primary as well.
> 
> Well, userspace still has to know how the src and dst coordinates relate to each other.
> 
> Src coordinates are based on the memory layout. So src x=0,y=0 is the first pixel in memory. x=1,y=0 is the second pixel in memory etc.
> 
> Dst coordinates are based on the display pipe. So dst x=0,y=0 is the first pixel that gets pushed out by the pipe for each frame. x=1,y=0 is the second pixel getting pushed out.
> 
> Let's say we have a 4x2 FB and a display mode of 8x4. Then we want to use a sprite plane to present the FB with 0 and 90 degree rotation, and we want to position the sprite plane at coordinates 2,0 on the CRTC.
> It should look something like this:
> 
> rotation=0
> src: x1=0,y1=0,x2=4,y2=2 (w=4, h=2)
> dst: x1=2,y1=0,x2=6,y2=2 (w=4, h=2)
> FB:            CRTC:
> 0,0 ----    0,0 --------
>    |abcd|      |  abcd  |
>    |efgh|      |  efgh  |
>     ---- 4,2   |        |
>                |        |
>                 -------- 8,4
> 
> rotation=90
> src=0,0 -> 4,2 (w=4, h=2)
> dst=2,0 -> 4,4 (w=2, h=4)
> FB:            CRTC:
> 0,0 ----    0,0 --------
>    |abcd|      |  dh    |
>    |efgh|      |  cg    |
>     ---- 4,2   |  bf    |
>                |  ae    |
>                 -------- 8,4
> 
> > >From the test, I don't really change the src sizes for primary as well as sprite to take care of 90/270 rotation.
> 
> Sounds a bit buggy then.
> 
> > 
> > -----Original Message-----
> > From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
> > Sent: Wednesday, January 14, 2015 5:14 PM
> > To: Jindal, Sonika
> > Cc: intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org
> > Subject: Re: [Intel-gfx] [PATCH 1/2] drm: Adding rotation to 
> > drm_plane_helper_check_update
> > 
> > On Wed, Jan 14, 2015 at 09:49:36AM +0000, Jindal, Sonika wrote:
> > > Since we do drm_rect_rotate with 90 rotation, the src->w changes to src->h.
> > > Now, when we call drm_rect_calc_hscale, the hscale calculated is lesser than the min_hscale (which is no_scaling by default), so it returns -ERANGE.
> > 
> > If you want no scaling then with 90/270 rotation then your src->w should be equal to dst->h. Then the calculated vscale will be 1.0. If it's not, then your test is passing in bad coordinates.
> > 
> > > So, I think we _relaxed is the function which should be called to update the destination size appropriately.
> > > Please correct me if I am wrong.
> > > 
> > > -----Original Message-----
> > > From: Jindal, Sonika
> > > Sent: Wednesday, January 14, 2015 3:06 PM
> > > To: 'Ville Syrjälä'
> > > Cc: intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org
> > > Subject: RE: [Intel-gfx] [PATCH 1/2] drm: Adding rotation to 
> > > drm_plane_helper_check_update
> > > 
> > > We do exactly like this for sprite plane (ie, rotate the rect, then check scaling and adjust the size accordingly from drm_rect_calc_hscale_relaxed) That's why I saw the need of this for primary plane as well. 
> > > For sprite plane 90 rotation, intel_check_sprite_plane does the adjustments and the rotated sizes are fine. But since we don't do any of those stuff for primary the destination size doesn't come right, and I get a little corrupted output after rotation.
> > > With this change, the rotated plane is properly adjusted in the viewport.
> > > So, I don't think it is a bug in test.
> > > 
> > > -----Original Message-----
> > > From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
> > > Sent: Wednesday, January 14, 2015 2:58 PM
> > > To: Jindal, Sonika
> > > Cc: intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org
> > > Subject: Re: [Intel-gfx] [PATCH 1/2] drm: Adding rotation to 
> > > drm_plane_helper_check_update
> > > 
> > > On Wed, Jan 14, 2015 at 10:05:53AM +0530, sonika wrote:
> > > > 
> > > > On Tuesday 13 January 2015 07:02 PM, Ville Syrjälä wrote:
> > > > > On Tue, Jan 13, 2015 at 06:03:39PM +0530, Sonika Jindal wrote:
> > > > >> Taking rotation into account while checking the plane and 
> > > > >> adjusting the sizes accordingly.
> > > > >>
> > > > >> Signed-off-by: Sonika Jindal <sonika.jindal@intel.com>
> > > > >> ---
> > > > >>   drivers/gpu/drm/drm_plane_helper.c |   79 ++++++++++++++++++++++++++++++++++--
> > > > >>   include/drm/drm_plane_helper.h     |    3 +-
> > > > >>   2 files changed, 77 insertions(+), 5 deletions(-)
> > > > >>
> > > > >> diff --git a/drivers/gpu/drm/drm_plane_helper.c
> > > > >> b/drivers/gpu/drm/drm_plane_helper.c
> > > > >> index f24c4cf..4badd69 100644
> > > > >> --- a/drivers/gpu/drm/drm_plane_helper.c
> > > > >> +++ b/drivers/gpu/drm/drm_plane_helper.c
> > > > >> @@ -138,9 +138,13 @@ int drm_plane_helper_check_update(struct drm_plane *plane,
> > > > >>   				    int max_scale,
> > > > >>   				    bool can_position,
> > > > >>   				    bool can_update_disabled,
> > > > >> -				    bool *visible)
> > > > >> +				    bool *visible,
> > > > >> +				    unsigned int rotation)
> > > > >>   {
> > > > >>   	int hscale, vscale;
> > > > >> +	int crtc_x, crtc_y;
> > > > >> +	unsigned int crtc_w, crtc_h;
> > > > >> +	uint32_t src_x, src_y, src_w, src_h;
> > > > >>   
> > > > >>   	if (!fb) {
> > > > >>   		*visible = false;
> > > > >> @@ -158,9 +162,13 @@ int drm_plane_helper_check_update(struct drm_plane *plane,
> > > > >>   		return -EINVAL;
> > > > >>   	}
> > > > >>   
> > > > >> +	if (fb)
> > > > >> +		drm_rect_rotate(src, fb->width << 16, fb->height << 16,
> > > > >> +				rotation);
> > > > >> +
> > > > >>   	/* Check scaling */
> > > > >> -	hscale = drm_rect_calc_hscale(src, dest, min_scale, max_scale);
> > > > >> -	vscale = drm_rect_calc_vscale(src, dest, min_scale, max_scale);
> > > > >> +	hscale = drm_rect_calc_hscale_relaxed(src, dest, min_scale, max_scale);
> > > > >> +	vscale = drm_rect_calc_vscale_relaxed(src, dest, min_scale, 
> > > > >> +max_scale);
> > > > > This is an unrelated change. Relaxed scaling allows the the 
> > > > > src/dest rectangles to be reduced in size in order to keep the 
> > > > > scaling ration within the min/max range. I suppose we should 
> > > > > switch to using it to make the behaviour uniform across drivers, 
> > > > > but definitely should be done with a separate patch.
> > > > Since, I added drm_rect_rotate before this, it changes the src 
> > > > sizes and it was giving me Invalid scaling if we don't let the 
> > > > sizes to be changed using _relaxed functions. I am trying this for 
> > > > 90/270 rotation.
> > > 
> > > That would indicate a bug somewhere. Pontetially the bug could be in whatever test you're using as well.
> > > 
> > > > I can move it to a separate patch if required.
> > > 
> > > We nee to figure out why you got the error first.
> > > 
> > > --
> > > Ville Syrjälä
> > > Intel OTC
> > 
> > --
> > Ville Syrjälä
> > Intel OTC
> 
> --
> Ville Syrjälä
> Intel OTC

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel