* [PATCH] drm/i915: Fix SKL+ 90/270 degree rotated plane coordinate computation
@ 2016-10-24 16:13 ville.syrjala
2016-10-24 16:46 ` ✗ Fi.CI.BAT: warning for " Patchwork
` (3 more replies)
0 siblings, 4 replies; 13+ messages in thread
From: ville.syrjala @ 2016-10-24 16:13 UTC (permalink / raw)
To: intel-gfx; +Cc: drm-intel-fixes
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
^ permalink raw reply related [flat|nested] 13+ messages in thread
* ✗ Fi.CI.BAT: warning for drm/i915: Fix SKL+ 90/270 degree rotated plane coordinate computation
2016-10-24 16:13 [PATCH] drm/i915: Fix SKL+ 90/270 degree rotated plane coordinate computation ville.syrjala
@ 2016-10-24 16:46 ` Patchwork
2016-10-24 17:40 ` Ville Syrjälä
2016-10-25 5:44 ` Saarinen, Jani
2016-10-25 7:20 ` [PATCH] " Chris Wilson
` (2 subsequent siblings)
3 siblings, 2 replies; 13+ messages in thread
From: Patchwork @ 2016-10-24 16:46 UTC (permalink / raw)
To: ville.syrjala; +Cc: intel-gfx
== 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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: ✗ Fi.CI.BAT: warning for drm/i915: Fix SKL+ 90/270 degree rotated plane coordinate computation
2016-10-24 16:46 ` ✗ Fi.CI.BAT: warning for " Patchwork
@ 2016-10-24 17:40 ` Ville Syrjälä
2016-10-25 5:44 ` Saarinen, Jani
1 sibling, 0 replies; 13+ messages in thread
From: Ville Syrjälä @ 2016-10-24 17:40 UTC (permalink / raw)
To: intel-gfx
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: ✗ Fi.CI.BAT: warning for drm/i915: Fix SKL+ 90/270 degree rotated plane coordinate computation
2016-10-24 16:46 ` ✗ Fi.CI.BAT: warning for " Patchwork
2016-10-24 17:40 ` Ville Syrjälä
@ 2016-10-25 5:44 ` Saarinen, Jani
1 sibling, 0 replies; 13+ messages in thread
From: Saarinen, Jani @ 2016-10-25 5:44 UTC (permalink / raw)
To: intel-gfx, ville.syrjala
> == 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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] drm/i915: Fix SKL+ 90/270 degree rotated plane coordinate computation
2016-10-24 16:13 [PATCH] drm/i915: Fix SKL+ 90/270 degree rotated plane coordinate computation ville.syrjala
2016-10-24 16:46 ` ✗ Fi.CI.BAT: warning for " Patchwork
@ 2016-10-25 7:20 ` Chris Wilson
2016-10-25 9:39 ` Ville Syrjälä
2016-10-25 14:04 ` Tvrtko Ursulin
2016-10-25 19:02 ` Paulo Zanoni
3 siblings, 1 reply; 13+ messages in thread
From: Chris Wilson @ 2016-10-25 7:20 UTC (permalink / raw)
To: ville.syrjala; +Cc: intel-gfx, drm-intel-fixes
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] drm/i915: Fix SKL+ 90/270 degree rotated plane coordinate computation
2016-10-25 7:20 ` [PATCH] " Chris Wilson
@ 2016-10-25 9:39 ` Ville Syrjälä
2016-10-25 9:55 ` Chris Wilson
0 siblings, 1 reply; 13+ messages in thread
From: Ville Syrjälä @ 2016-10-25 9:39 UTC (permalink / raw)
To: Chris Wilson, intel-gfx, drm-intel-fixes
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] drm/i915: Fix SKL+ 90/270 degree rotated plane coordinate computation
2016-10-25 9:39 ` Ville Syrjälä
@ 2016-10-25 9:55 ` Chris Wilson
2016-10-26 15:17 ` Ville Syrjälä
2016-10-26 17:39 ` Ville Syrjälä
0 siblings, 2 replies; 13+ messages in thread
From: Chris Wilson @ 2016-10-25 9:55 UTC (permalink / raw)
To: Ville Syrjälä; +Cc: intel-gfx, drm-intel-fixes
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] drm/i915: Fix SKL+ 90/270 degree rotated plane coordinate computation
2016-10-24 16:13 [PATCH] drm/i915: Fix SKL+ 90/270 degree rotated plane coordinate computation ville.syrjala
2016-10-24 16:46 ` ✗ Fi.CI.BAT: warning for " Patchwork
2016-10-25 7:20 ` [PATCH] " Chris Wilson
@ 2016-10-25 14:04 ` Tvrtko Ursulin
2016-10-25 19:02 ` Paulo Zanoni
3 siblings, 0 replies; 13+ messages in thread
From: Tvrtko Ursulin @ 2016-10-25 14:04 UTC (permalink / raw)
To: ville.syrjala, intel-gfx; +Cc: drm-intel-fixes
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] drm/i915: Fix SKL+ 90/270 degree rotated plane coordinate computation
2016-10-24 16:13 [PATCH] drm/i915: Fix SKL+ 90/270 degree rotated plane coordinate computation ville.syrjala
` (2 preceding siblings ...)
2016-10-25 14:04 ` Tvrtko Ursulin
@ 2016-10-25 19:02 ` Paulo Zanoni
2016-10-26 6:25 ` Daniel Vetter
2016-10-26 15:04 ` Ville Syrjälä
3 siblings, 2 replies; 13+ messages in thread
From: Paulo Zanoni @ 2016-10-25 19:02 UTC (permalink / raw)
To: ville.syrjala, intel-gfx; +Cc: drm-intel-fixes
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] drm/i915: Fix SKL+ 90/270 degree rotated plane coordinate computation
2016-10-25 19:02 ` Paulo Zanoni
@ 2016-10-26 6:25 ` Daniel Vetter
2016-10-26 15:04 ` Ville Syrjälä
1 sibling, 0 replies; 13+ messages in thread
From: Daniel Vetter @ 2016-10-26 6:25 UTC (permalink / raw)
To: Paulo Zanoni; +Cc: intel-gfx, drm-intel-fixes
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] drm/i915: Fix SKL+ 90/270 degree rotated plane coordinate computation
2016-10-25 19:02 ` Paulo Zanoni
2016-10-26 6:25 ` Daniel Vetter
@ 2016-10-26 15:04 ` Ville Syrjälä
1 sibling, 0 replies; 13+ messages in thread
From: Ville Syrjälä @ 2016-10-26 15:04 UTC (permalink / raw)
To: Paulo Zanoni; +Cc: intel-gfx, drm-intel-fixes
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] drm/i915: Fix SKL+ 90/270 degree rotated plane coordinate computation
2016-10-25 9:55 ` Chris Wilson
@ 2016-10-26 15:17 ` Ville Syrjälä
2016-10-26 17:39 ` Ville Syrjälä
1 sibling, 0 replies; 13+ messages in thread
From: Ville Syrjälä @ 2016-10-26 15:17 UTC (permalink / raw)
To: Chris Wilson, intel-gfx, drm-intel-fixes
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] drm/i915: Fix SKL+ 90/270 degree rotated plane coordinate computation
2016-10-25 9:55 ` Chris Wilson
2016-10-26 15:17 ` Ville Syrjälä
@ 2016-10-26 17:39 ` Ville Syrjälä
1 sibling, 0 replies; 13+ messages in thread
From: Ville Syrjälä @ 2016-10-26 17:39 UTC (permalink / raw)
To: Chris Wilson, intel-gfx, drm-intel-fixes
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
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2016-10-26 17:39 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-10-24 16:13 [PATCH] drm/i915: Fix SKL+ 90/270 degree rotated plane coordinate computation ville.syrjala
2016-10-24 16:46 ` ✗ Fi.CI.BAT: warning for " Patchwork
2016-10-24 17:40 ` Ville Syrjälä
2016-10-25 5:44 ` Saarinen, Jani
2016-10-25 7:20 ` [PATCH] " Chris Wilson
2016-10-25 9:39 ` Ville Syrjälä
2016-10-25 9:55 ` Chris Wilson
2016-10-26 15:17 ` Ville Syrjälä
2016-10-26 17:39 ` Ville Syrjälä
2016-10-25 14:04 ` Tvrtko Ursulin
2016-10-25 19:02 ` Paulo Zanoni
2016-10-26 6:25 ` Daniel Vetter
2016-10-26 15:04 ` Ville Syrjälä
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.