[PATCH] drm/i915: Reject non-canonical rotations

[PATCH] drm/i915: Reject non-canonical rotations

[Matthew Auld]

Reject any rotation value which incorrectly represents multiple rotations.

Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Signed-off-by: Matthew Auld <matthew.auld@intel.com>
---
 drivers/gpu/drm/i915/intel_atomic_plane.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
index 7de7721..6cb564f 100644
--- a/drivers/gpu/drm/i915/intel_atomic_plane.c
+++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
@@ -156,6 +156,11 @@ static int intel_plane_atomic_check(struct drm_plane *plane,
 	intel_state->clip.y2 =
 		crtc_state->base.enable ? crtc_state->pipe_src_h : 0;
 
+	if (!is_power_of_2(state->rotation)) {
+		DRM_DEBUG_KMS("Multiple rotations are not supported!\n");
+		return -EINVAL;
+	}
+
 	if (state->fb && intel_rotation_90_or_270(state->rotation)) {
 		if (!(state->fb->modifier[0] == I915_FORMAT_MOD_Y_TILED ||
 			state->fb->modifier[0] == I915_FORMAT_MOD_Yf_TILED)) {
-- 
2.4.3

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

Re: [PATCH] drm/i915: Reject non-canonical rotations

[Ville Syrjälä]

On Tue, Mar 22, 2016 at 10:35:41AM +0000, Matthew Auld wrote:
> Reject any rotation value which incorrectly represents multiple rotations.
> 
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Signed-off-by: Matthew Auld <matthew.auld@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_atomic_plane.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
> index 7de7721..6cb564f 100644
> --- a/drivers/gpu/drm/i915/intel_atomic_plane.c
> +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
> @@ -156,6 +156,11 @@ static int intel_plane_atomic_check(struct drm_plane *plane,
>  	intel_state->clip.y2 =
>  		crtc_state->base.enable ? crtc_state->pipe_src_h : 0;
>  
> +	if (!is_power_of_2(state->rotation)) {
> +		DRM_DEBUG_KMS("Multiple rotations are not supported!\n");
> +		return -EINVAL;
> +	}

Such a check should be done in the core. Are we not doing it there?

> +
>  	if (state->fb && intel_rotation_90_or_270(state->rotation)) {
>  		if (!(state->fb->modifier[0] == I915_FORMAT_MOD_Y_TILED ||
>  			state->fb->modifier[0] == I915_FORMAT_MOD_Yf_TILED)) {
> -- 
> 2.4.3
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
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: Reject non-canonical rotations

[Matthew Auld]

As in the drm core? Not as far as I could tell...
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Reject non-canonical rotations

[Ville Syrjälä]

On Tue, Mar 22, 2016 at 10:48:41AM +0000, Matthew Auld wrote:
> As in the drm core? Not as far as I could tell...

A good time to add it then I guess ;)

-- 
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: Reject non-canonical rotations

[Daniel Vetter]

On Tue, Mar 22, 2016 at 12:59:19PM +0200, Ville Syrjälä wrote:
> On Tue, Mar 22, 2016 at 10:48:41AM +0000, Matthew Auld wrote:
> > As in the drm core? Not as far as I could tell...
> 
> A good time to add it then I guess ;)

I thought we do normalize this somewhere. In other words your static code
analyser didn't read the code well enough probably ;-)

On that topic: Your patch lacks motivation. Yes I can usually guess when
it's due to static analyzer checks, but you need to explain that. And you
need to explain what exactly the analyzer is complaining about.

There's some conflicting opinions about whether you're allowed to name the
tool itself, I personally don't care much but would appreciate those
details too. But the details of what the static analyzer discovered and
_must_ be in the commit message. Otherwise no way to review whether your
patch fixes the problem in a reasonable way.

This means please resend your entire pile of recent submission.

Thanks, 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

✗ Fi.CI.BAT: warning for drm/i915: Reject non-canonical rotations

[Patchwork]

== Series Details ==

Series: drm/i915: Reject non-canonical rotations
URL   : https://patchwork.freedesktop.org/series/4746/
State : warning

== Summary ==

Series 4746v1 drm/i915: Reject non-canonical rotations
http://patchwork.freedesktop.org/api/1.0/series/4746/revisions/1/mbox/

Test gem_exec_suspend:
        Subgroup basic-s3:
                pass       -> DMESG-WARN (bsw-nuc-2)
Test kms_flip:
        Subgroup basic-flip-vs-dpms:
                dmesg-warn -> PASS       (ilk-hp8440p) UNSTABLE
        Subgroup basic-plain-flip:
                pass       -> DMESG-WARN (hsw-brixbox)
                dmesg-warn -> PASS       (bdw-ultra)
Test kms_pipe_crc_basic:
        Subgroup read-crc-pipe-b-frame-sequence:
                pass       -> DMESG-WARN (snb-x220t)
        Subgroup suspend-read-crc-pipe-c:
                dmesg-warn -> PASS       (bsw-nuc-2)
Test pm_rpm:
        Subgroup basic-pci-d3-state:
                pass       -> DMESG-WARN (snb-dellxps)
                pass       -> DMESG-WARN (byt-nuc)
        Subgroup basic-rte:
                dmesg-warn -> PASS       (snb-x220t)
                dmesg-warn -> PASS       (byt-nuc) UNSTABLE

bdw-nuci7        total:192  pass:180  dwarn:0   dfail:0   fail:0   skip:12 
bdw-ultra        total:192  pass:171  dwarn:0   dfail:0   fail:0   skip:21 
bsw-nuc-2        total:192  pass:154  dwarn:1   dfail:0   fail:0   skip:37 
byt-nuc          total:192  pass:156  dwarn:1   dfail:0   fail:0   skip:35 
hsw-brixbox      total:192  pass:169  dwarn:1   dfail:0   fail:0   skip:22 
ilk-hp8440p      total:192  pass:129  dwarn:0   dfail:0   fail:0   skip:63 
ivb-t430s        total:192  pass:167  dwarn:0   dfail:0   fail:0   skip:25 
skl-i5k-2        total:192  pass:169  dwarn:0   dfail:0   fail:0   skip:23 
skl-i7k-2        total:192  pass:169  dwarn:0   dfail:0   fail:0   skip:23 
snb-dellxps      total:192  pass:156  dwarn:2   dfail:0   fail:0   skip:34 
snb-x220t        total:192  pass:157  dwarn:1   dfail:0   fail:1   skip:33 

Results at /archive/results/CI_IGT_test/Patchwork_1673/

4b39223f6e3bef4dfa678f7239dcd87c38e20e96 drm-intel-nightly: 2016y-03m-21d-18h-43m-18s UTC integration manifest
8fb43d81a892947726fdeff37e1ffbd524ab8154 drm/i915: Reject non-canonical rotations

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

Re: [PATCH] drm/i915: Reject non-canonical rotations

[Matthew Auld]

Hi Daniel,

>> I thought we do normalize this somewhere.

I did write an i-g-t test which submits such a rotation value and it
is not rejected.

>> Your patch lacks motivation

As in I haven't properly conveyed the motivation behind the patch in
the commit message?

>> Yes I can usually guess when
it's due to static analyzer checks, but you need to explain that. And you
need to explain what exactly the analyzer is complaining about.

erm, no static analyser, for this patch or any prior, promise, but duly noted ;)

Joonas actually suggested this patch, and some of the preceding ones
as beginner tasks for me.

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

Re: [PATCH] drm/i915: Reject non-canonical rotations

[Daniel Vetter]

On Tue, Mar 22, 2016 at 02:14:38PM +0000, Matthew Auld wrote:
> Hi Daniel,
> 
> >> I thought we do normalize this somewhere.
> 
> I did write an i-g-t test which submits such a rotation value and it
> is not rejected.

Normalize = the drm core makes sure drivers don't see all the
combinations, but only canonical values. Userspace can still submit values
with too many bits set. I'm not sure we want (or can, it's ABI) change
that.

> >> Your patch lacks motivation
> 
> As in I haven't properly conveyed the motivation behind the patch in
> the commit message?
> 
> >> Yes I can usually guess when
> it's due to static analyzer checks, but you need to explain that. And you
> need to explain what exactly the analyzer is complaining about.
> 
> erm, no static analyser, for this patch or any prior, promise, but duly noted ;)
> 
> Joonas actually suggested this patch, and some of the preceding ones
> as beginner tasks for me.

Oh surprising, spotting all these random things all over tends to be stuff
only static analyzers manage ;-) Patch still needs some motivation, since
if your igt passes and the driver behaves correctly it's all fine.
-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: Reject non-canonical rotations

[Joonas Lahtinen]

On ke, 2016-03-23 at 09:58 +0100, Daniel Vetter wrote:
> On Tue, Mar 22, 2016 at 02:14:38PM +0000, Matthew Auld wrote:
> > 
> > Hi Daniel,
> > 
> > > 
> > > > 
> > > > I thought we do normalize this somewhere.
> > I did write an i-g-t test which submits such a rotation value and it
> > is not rejected.
> Normalize = the drm core makes sure drivers don't see all the
> combinations, but only canonical values. Userspace can still submit values
> with too many bits set. I'm not sure we want (or can, it's ABI) change
> that.
> 
> > 
> > > 
> > > > 
> > > > Your patch lacks motivation
> > As in I haven't properly conveyed the motivation behind the patch in
> > the commit message?
> > 
> > > 
> > > > 
> > > > Yes I can usually guess when
> > it's due to static analyzer checks, but you need to explain that. And you
> > need to explain what exactly the analyzer is complaining about.
> > 
> > erm, no static analyser, for this patch or any prior, promise, but duly noted ;)
> > 
> > Joonas actually suggested this patch, and some of the preceding ones
> > as beginner tasks for me.
> Oh surprising, spotting all these random things all over tends to be stuff
> only static analyzers manage ;-) Patch still needs some motivation, since
> if your igt passes and the driver behaves correctly it's all fine.

I'm happy to mention that the motivation this was on my backlog is that
it was *YOU* who asked me to implement it along with the IGT tests :P

But I guess, now if it's implemented in DRM layer already, matter of
making sure the kms_rotation_crc tests the current expected behaviour.

And by what you described (drivers won't see bad values, ABI accepts
them), it would mean to just attempt to send invalid combinations, they
should be happily accepted but resulting rotation will be undefined. I
myself would reject invalid bit combinations, but if the ABI has
already grown that way, not much to be done at this point.

Regards, Joonas

> -Daniel
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Reject non-canonical rotations

[Daniel Vetter]

On Wed, Mar 23, 2016 at 03:30:48PM +0200, Joonas Lahtinen wrote:
> On ke, 2016-03-23 at 09:58 +0100, Daniel Vetter wrote:
> > On Tue, Mar 22, 2016 at 02:14:38PM +0000, Matthew Auld wrote:
> > > 
> > > Hi Daniel,
> > > 
> > > > 
> > > > > 
> > > > > I thought we do normalize this somewhere.
> > > I did write an i-g-t test which submits such a rotation value and it
> > > is not rejected.
> > Normalize = the drm core makes sure drivers don't see all the
> > combinations, but only canonical values. Userspace can still submit values
> > with too many bits set. I'm not sure we want (or can, it's ABI) change
> > that.
> > 
> > > 
> > > > 
> > > > > 
> > > > > Your patch lacks motivation
> > > As in I haven't properly conveyed the motivation behind the patch in
> > > the commit message?
> > > 
> > > > 
> > > > > 
> > > > > Yes I can usually guess when
> > > it's due to static analyzer checks, but you need to explain that. And you
> > > need to explain what exactly the analyzer is complaining about.
> > > 
> > > erm, no static analyser, for this patch or any prior, promise, but duly noted ;)
> > > 
> > > Joonas actually suggested this patch, and some of the preceding ones
> > > as beginner tasks for me.
> > Oh surprising, spotting all these random things all over tends to be stuff
> > only static analyzers manage ;-) Patch still needs some motivation, since
> > if your igt passes and the driver behaves correctly it's all fine.
> 
> I'm happy to mention that the motivation this was on my backlog is that
> it was *YOU* who asked me to implement it along with the IGT tests :P
> 
> But I guess, now if it's implemented in DRM layer already, matter of
> making sure the kms_rotation_crc tests the current expected behaviour.
> 
> And by what you described (drivers won't see bad values, ABI accepts
> them), it would mean to just attempt to send invalid combinations, they
> should be happily accepted but resulting rotation will be undefined. I
> myself would reject invalid bit combinations, but if the ABI has
> already grown that way, not much to be done at this point.

Well so I unlazied and actually read the code. We do have the helper
function in drm_rotation_simplify, but it's not called. So still work to
do.
-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: Reject non-canonical rotations

[Matthew Auld]

Okay, I'll rework the patch to use drm_rotation_simplify instead.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Reject non-canonical rotations

[Ville Syrjälä]

On Wed, Mar 23, 2016 at 05:18:08PM +0100, Daniel Vetter wrote:
> On Wed, Mar 23, 2016 at 03:30:48PM +0200, Joonas Lahtinen wrote:
> > On ke, 2016-03-23 at 09:58 +0100, Daniel Vetter wrote:
> > > On Tue, Mar 22, 2016 at 02:14:38PM +0000, Matthew Auld wrote:
> > > > 
> > > > Hi Daniel,
> > > > 
> > > > > 
> > > > > > 
> > > > > > I thought we do normalize this somewhere.
> > > > I did write an i-g-t test which submits such a rotation value and it
> > > > is not rejected.
> > > Normalize = the drm core makes sure drivers don't see all the
> > > combinations, but only canonical values. Userspace can still submit values
> > > with too many bits set. I'm not sure we want (or can, it's ABI) change
> > > that.
> > > 
> > > > 
> > > > > 
> > > > > > 
> > > > > > Your patch lacks motivation
> > > > As in I haven't properly conveyed the motivation behind the patch in
> > > > the commit message?
> > > > 
> > > > > 
> > > > > > 
> > > > > > Yes I can usually guess when
> > > > it's due to static analyzer checks, but you need to explain that. And you
> > > > need to explain what exactly the analyzer is complaining about.
> > > > 
> > > > erm, no static analyser, for this patch or any prior, promise, but duly noted ;)
> > > > 
> > > > Joonas actually suggested this patch, and some of the preceding ones
> > > > as beginner tasks for me.
> > > Oh surprising, spotting all these random things all over tends to be stuff
> > > only static analyzers manage ;-) Patch still needs some motivation, since
> > > if your igt passes and the driver behaves correctly it's all fine.
> > 
> > I'm happy to mention that the motivation this was on my backlog is that
> > it was *YOU* who asked me to implement it along with the IGT tests :P
> > 
> > But I guess, now if it's implemented in DRM layer already, matter of
> > making sure the kms_rotation_crc tests the current expected behaviour.
> > 
> > And by what you described (drivers won't see bad values, ABI accepts
> > them), it would mean to just attempt to send invalid combinations, they
> > should be happily accepted but resulting rotation will be undefined. I
> > myself would reject invalid bit combinations, but if the ABI has
> > already grown that way, not much to be done at this point.
> 
> Well so I unlazied and actually read the code. We do have the helper
> function in drm_rotation_simplify, but it's not called. So still work to
> do.

That's not related to rejecting invalid bit combinations. It's meant for
the case where the hardware implements, say, all rotation angles and one
reflection, or 0+90 and both reflections. By using
drm_rotation_simplify() the driver can just deal with the bits that the
hardware actually implements while we still expose everything to userspace.

The core should in any case reject the multiple rotation bits set at
the same time thing. We had that code in the i915 set_property code but
it was lost in some atomic conversion.

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