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

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

[ville.syrjala]

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.

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
-- 
2.7.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.BAT: warning for drm/i915: Fix SKL+ 90/270 degree rotated plane coordinate computation

[Patchwork]

== Series Details ==

Series: drm/i915: Fix SKL+ 90/270 degree rotated plane coordinate computation
URL   : https://patchwork.freedesktop.org/series/14279/
State : warning

== Summary ==

Series 14279v1 drm/i915: Fix SKL+ 90/270 degree rotated plane coordinate computation
https://patchwork.freedesktop.org/api/1.0/series/14279/revisions/1/mbox/

Test drv_module_reload_basic:
                pass       -> SKIP       (fi-skl-6260u)
Test kms_pipe_crc_basic:
        Subgroup nonblocking-crc-pipe-a:
                pass       -> DMESG-WARN (fi-ilk-650)

fi-bdw-5557u     total:246  pass:231  dwarn:0   dfail:0   fail:0   skip:15 
fi-bsw-n3050     total:246  pass:204  dwarn:0   dfail:0   fail:0   skip:42 
fi-bxt-t5700     total:246  pass:216  dwarn:0   dfail:0   fail:0   skip:30 
fi-byt-j1900     total:246  pass:215  dwarn:0   dfail:0   fail:0   skip:31 
fi-byt-n2820     total:246  pass:211  dwarn:0   dfail:0   fail:0   skip:35 
fi-hsw-4770      total:246  pass:224  dwarn:0   dfail:0   fail:0   skip:22 
fi-hsw-4770r     total:246  pass:224  dwarn:0   dfail:0   fail:0   skip:22 
fi-ilk-650       total:246  pass:184  dwarn:1   dfail:0   fail:1   skip:60 
fi-ivb-3520m     total:246  pass:221  dwarn:0   dfail:0   fail:0   skip:25 
fi-ivb-3770      total:246  pass:221  dwarn:0   dfail:0   fail:0   skip:25 
fi-skl-6260u     total:246  pass:231  dwarn:0   dfail:0   fail:0   skip:15 
fi-skl-6700hq    total:246  pass:219  dwarn:4   dfail:0   fail:0   skip:23 
fi-skl-6700k     total:246  pass:222  dwarn:1   dfail:0   fail:0   skip:23 
fi-skl-6770hq    total:246  pass:232  dwarn:0   dfail:0   fail:0   skip:14 
fi-snb-2520m     total:246  pass:210  dwarn:0   dfail:0   fail:0   skip:36 
fi-snb-2600      total:246  pass:209  dwarn:0   dfail:0   fail:0   skip:37 

Results at /Patchwork_2800/

102441087e47f4892b88c4f6e308e3edf89eeda1 drm-intel-nightly: 2016y-10m-24d-14h-28m-17s UTC integration manifest
b8b2df0 drm/i915: Fix SKL+ 90/270 degree rotated plane coordinate computation

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: ✗ Fi.CI.BAT: warning for drm/i915: Fix SKL+ 90/270 degree rotated plane coordinate computation

[Ville Syrjälä]

On Mon, Oct 24, 2016 at 04:46:00PM -0000, Patchwork wrote:
> == Series Details ==
> 
> Series: drm/i915: Fix SKL+ 90/270 degree rotated plane coordinate computation
> URL   : https://patchwork.freedesktop.org/series/14279/
> State : warning
> 
> == Summary ==
> 
> Series 14279v1 drm/i915: Fix SKL+ 90/270 degree rotated plane coordinate computation
> https://patchwork.freedesktop.org/api/1.0/series/14279/revisions/1/mbox/
> 
> Test drv_module_reload_basic:
>                 pass       -> SKIP       (fi-skl-6260u)

rmmod: ERROR: Module snd_hda_intel is in use

> Test kms_pipe_crc_basic:
>         Subgroup nonblocking-crc-pipe-a:
>                 pass       -> DMESG-WARN (fi-ilk-650)

[  343.396199] [drm:intel_dp_check_link_status [i915]] DP C: channel EQ not ok, retraining
[  343.396575] [drm:intel_dp_set_signal_levels [i915]] Using signal levels 00000000
[  343.396591] [drm:intel_dp_set_signal_levels [i915]] Using vswing level 0
[  343.396606] [drm:intel_dp_set_signal_levels [i915]] Using pre-emphasis level 0
[  343.396624] [drm:intel_dp_program_link_training_pattern [i915]] Using DP training pattern TPS1
[  343.397314] [drm:intel_dp_set_signal_levels [i915]] Using signal levels 02000000
[  343.397331] [drm:intel_dp_set_signal_levels [i915]] Using vswing level 1
[  343.397346] [drm:intel_dp_set_signal_levels [i915]] Using pre-emphasis level 0
[  343.398049] [drm:intel_dp_set_signal_levels [i915]] Using signal levels 04000000
[  343.398065] [drm:intel_dp_set_signal_levels [i915]] Using vswing level 2
[  343.398080] [drm:intel_dp_set_signal_levels [i915]] Using pre-emphasis level 0
[  343.398797] [drm:intel_dp_start_link_train [i915]] Max Voltage Swing reached
[  343.398841] [drm:intel_dp_program_link_training_pattern [i915]] Using DP training pattern TPS2
[  343.399910] [drm:intel_dp_dump_link_status [i915]] ln0_1:0x0 ln2_3:0x0 align:0x80 sink:0x0 adj_req0_1:0x77 adj_req2_3:0x77
[  343.399927] [drm:intel_dp_start_link_train [i915]] Clock recovery check failed, cannot continue channel equalization...
[  343.417162] [drm:intel_pch_fifo_underrun_irq_handler [i915]] *ERROR* PCH transcoder B FIFO underrun

/me thinks we need to pimp our link retraining a bit.

> 
> fi-bdw-5557u     total:246  pass:231  dwarn:0   dfail:0   fail:0   skip:15 
> fi-bsw-n3050     total:246  pass:204  dwarn:0   dfail:0   fail:0   skip:42 
> fi-bxt-t5700     total:246  pass:216  dwarn:0   dfail:0   fail:0   skip:30 
> fi-byt-j1900     total:246  pass:215  dwarn:0   dfail:0   fail:0   skip:31 
> fi-byt-n2820     total:246  pass:211  dwarn:0   dfail:0   fail:0   skip:35 
> fi-hsw-4770      total:246  pass:224  dwarn:0   dfail:0   fail:0   skip:22 
> fi-hsw-4770r     total:246  pass:224  dwarn:0   dfail:0   fail:0   skip:22 
> fi-ilk-650       total:246  pass:184  dwarn:1   dfail:0   fail:1   skip:60 
> fi-ivb-3520m     total:246  pass:221  dwarn:0   dfail:0   fail:0   skip:25 
> fi-ivb-3770      total:246  pass:221  dwarn:0   dfail:0   fail:0   skip:25 
> fi-skl-6260u     total:246  pass:231  dwarn:0   dfail:0   fail:0   skip:15 
> fi-skl-6700hq    total:246  pass:219  dwarn:4   dfail:0   fail:0   skip:23 
> fi-skl-6700k     total:246  pass:222  dwarn:1   dfail:0   fail:0   skip:23 
> fi-skl-6770hq    total:246  pass:232  dwarn:0   dfail:0   fail:0   skip:14 
> fi-snb-2520m     total:246  pass:210  dwarn:0   dfail:0   fail:0   skip:36 
> fi-snb-2600      total:246  pass:209  dwarn:0   dfail:0   fail:0   skip:37 
> 
> Results at /Patchwork_2800/
> 
> 102441087e47f4892b88c4f6e308e3edf89eeda1 drm-intel-nightly: 2016y-10m-24d-14h-28m-17s UTC integration manifest
> b8b2df0 drm/i915: Fix SKL+ 90/270 degree rotated plane coordinate computation

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

Re: ✗ Fi.CI.BAT: warning for drm/i915: Fix SKL+ 90/270 degree rotated plane coordinate computation

[Saarinen, Jani]

> == Series Details ==
> 
> Series: drm/i915: Fix SKL+ 90/270 degree rotated plane coordinate computation
> URL   : https://patchwork.freedesktop.org/series/14279/
> State : warning
> 
> == Summary ==
> 
> Series 14279v1 drm/i915: Fix SKL+ 90/270 degree rotated plane coordinate
> computation
> https://patchwork.freedesktop.org/api/1.0/series/14279/revisions/1/mbox/
> 
> Test drv_module_reload_basic:
>                 pass       -> SKIP       (fi-skl-6260u)
Out	
Reloading i915.ko with
unbinding /sys/class/vtconsole/vtcon1/: (M) frame buffer device
module successfully unloaded
module successfully loaded again
Reloading i915.ko with inject_load_failure=1
unbinding /sys/class/vtconsole/vtcon1/: (M) frame buffer device
Reloading i915.ko with inject_load_failure=2
unbinding /sys/class/vtconsole/vtcon1/: (M) frame buffer device
Reloading i915.ko with inject_load_failure=3
unbinding /sys/class/vtconsole/vtcon1/: (M) frame buffer device
Reloading i915.ko with inject_load_failure=4
unbinding /sys/class/vtconsole/vtcon1/: (M) frame buffer device
Reloading i915.ko with
unbinding /sys/class/vtconsole/vtcon1/: (M) frame buffer device

Err	
rmmod: ERROR: Module snd_hda_intel is in use
Module                  Size  Used by
snd_hda_intel          30473  1

Audio team looking at this.

> Test kms_pipe_crc_basic:
>         Subgroup nonblocking-crc-pipe-a:
>                 pass       -> DMESG-WARN (fi-ilk-650)
https://bugs.freedesktop.org/show_bug.cgi?id=98251

> 
> fi-bdw-5557u     total:246  pass:231  dwarn:0   dfail:0   fail:0   skip:15
> fi-bsw-n3050     total:246  pass:204  dwarn:0   dfail:0   fail:0   skip:42
> fi-bxt-t5700     total:246  pass:216  dwarn:0   dfail:0   fail:0   skip:30
> fi-byt-j1900     total:246  pass:215  dwarn:0   dfail:0   fail:0   skip:31
> fi-byt-n2820     total:246  pass:211  dwarn:0   dfail:0   fail:0   skip:35
> fi-hsw-4770      total:246  pass:224  dwarn:0   dfail:0   fail:0   skip:22
> fi-hsw-4770r     total:246  pass:224  dwarn:0   dfail:0   fail:0   skip:22
> fi-ilk-650       total:246  pass:184  dwarn:1   dfail:0   fail:1   skip:60
> fi-ivb-3520m     total:246  pass:221  dwarn:0   dfail:0   fail:0   skip:25
> fi-ivb-3770      total:246  pass:221  dwarn:0   dfail:0   fail:0   skip:25
> fi-skl-6260u     total:246  pass:231  dwarn:0   dfail:0   fail:0   skip:15
> fi-skl-6700hq    total:246  pass:219  dwarn:4   dfail:0   fail:0   skip:23
> fi-skl-6700k     total:246  pass:222  dwarn:1   dfail:0   fail:0   skip:23
> fi-skl-6770hq    total:246  pass:232  dwarn:0   dfail:0   fail:0   skip:14
> fi-snb-2520m     total:246  pass:210  dwarn:0   dfail:0   fail:0   skip:36
> fi-snb-2600      total:246  pass:209  dwarn:0   dfail:0   fail:0   skip:37
> 
> Results at /Patchwork_2800/
> 
> 102441087e47f4892b88c4f6e308e3edf89eeda1 drm-intel-nightly: 2016y-10m-
> 24d-14h-28m-17s UTC integration manifest
> b8b2df0 drm/i915: Fix SKL+ 90/270 degree rotated plane coordinate
> computation


Jani Saarinen
Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Chris Wilson]

On Mon, Oct 24, 2016 at 07:13:04PM +0300, ville.syrjala@linux.intel.com wrote:
> 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.
> 
> 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);

Line 2576 (intel_fill_fb_info()) also looks wrong.

drm_rect_rotate(&r,
		rot_info->plane[i].width * tile_width,
		rot_info->plane[i].height * tile_height,
		DRM_ROTATE_270);
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Ville Syrjälä]

On Tue, Oct 25, 2016 at 08:20:46AM +0100, Chris Wilson wrote:
> On Mon, Oct 24, 2016 at 07:13:04PM +0300, ville.syrjala@linux.intel.com wrote:
> > 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.
> > 
> > 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);
> 
> Line 2576 (intel_fill_fb_info()) also looks wrong.
> 
> drm_rect_rotate(&r,
> 		rot_info->plane[i].width * tile_width,
> 		rot_info->plane[i].height * tile_height,
> 		DRM_ROTATE_270);

That should be fine. No sub-pixel stuff going on in that
function.

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

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

[Chris Wilson]

On Tue, Oct 25, 2016 at 12:39:43PM +0300, Ville Syrjälä wrote:
> On Tue, Oct 25, 2016 at 08:20:46AM +0100, Chris Wilson wrote:
> > On Mon, Oct 24, 2016 at 07:13:04PM +0300, ville.syrjala@linux.intel.com wrote:
> > > 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.
> > > 
> > > 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);
> > 
> > Line 2576 (intel_fill_fb_info()) also looks wrong.
> > 
> > drm_rect_rotate(&r,
> > 		rot_info->plane[i].width * tile_width,
> > 		rot_info->plane[i].height * tile_height,
> > 		DRM_ROTATE_270);
> 
> That should be fine. No sub-pixel stuff going on in that
> function.

Ah, yes r is not scaled.

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

drm_plane_subpixel_scale(fb->width) ?
drm_plane_scale_pixels_to_subpixels(fb->width) ?

Not sure what terminology you are already using for the plane_state->src
coordinates.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Ville Syrjälä]

On Tue, Oct 25, 2016 at 10:55:35AM +0100, Chris Wilson wrote:
> On Tue, Oct 25, 2016 at 12:39:43PM +0300, Ville Syrjälä wrote:
> > On Tue, Oct 25, 2016 at 08:20:46AM +0100, Chris Wilson wrote:
> > > On Mon, Oct 24, 2016 at 07:13:04PM +0300, ville.syrjala@linux.intel.com wrote:
> > > > 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.
> > > > 
> > > > 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);
> > > 
> > > Line 2576 (intel_fill_fb_info()) also looks wrong.
> > > 
> > > drm_rect_rotate(&r,
> > > 		rot_info->plane[i].width * tile_width,
> > > 		rot_info->plane[i].height * tile_height,
> > > 		DRM_ROTATE_270);
> > 
> > That should be fine. No sub-pixel stuff going on in that
> > function.
> 
> Ah, yes r is not scaled.
> 
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> 
> drm_plane_subpixel_scale(fb->width) ?
> drm_plane_scale_pixels_to_subpixels(fb->width) ?

I guess we could have something like that. Can't gurarantee it wouldn't
confuse me though. As I replied to Paulo, my brain has a hard time
understanding abstracted fixed point stuff.

> 
> Not sure what terminology you are already using for the plane_state->src
> coordinates.

Me neither. Sometimes I refer to sub-pixel coordinates, sometimes
fractional coordinates, and sometimes perhaps even something else
that I can't recall right now.

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

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

[Ville Syrjälä]

On Tue, Oct 25, 2016 at 10:55:35AM +0100, Chris Wilson wrote:
> On Tue, Oct 25, 2016 at 12:39:43PM +0300, Ville Syrjälä wrote:
> > On Tue, Oct 25, 2016 at 08:20:46AM +0100, Chris Wilson wrote:
> > > On Mon, Oct 24, 2016 at 07:13:04PM +0300, ville.syrjala@linux.intel.com wrote:
> > > > 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.
> > > > 
> > > > 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);
> > > 
> > > Line 2576 (intel_fill_fb_info()) also looks wrong.
> > > 
> > > drm_rect_rotate(&r,
> > > 		rot_info->plane[i].width * tile_width,
> > > 		rot_info->plane[i].height * tile_height,
> > > 		DRM_ROTATE_270);
> > 
> > That should be fine. No sub-pixel stuff going on in that
> > function.
> 
> Ah, yes r is not scaled.
> 
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

Pushed to dinq. Thanks for the review and testing.

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

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

[Tvrtko Ursulin]

On 24/10/2016 17:13, ville.syrjala@linux.intel.com wrote:
> 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.
>
> 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
>

Tested-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Paulo Zanoni]

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 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Daniel Vetter]

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 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?

One ugliness is that we have struct drm_rect in 16_16 format, and others
as real integers. It would be fairly invasive I fear (touching lots of
drivers). But might be worth it. One intermediate step might be to just
add typedefs, since those can be casted silently. Still leaves the door
wide open for bugs, but at least it would serve as pretty good hints in
the code about what's going on.

Once we have the typedefs we could have the functions for rounding to
integers or converting integers to fixed point (same for drm_rect). And
once we have that we could exchange the typedefs for the opaque structs
like the one above.

Lots of work indeed ...
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Ville Syrjälä]

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