* [PATCH] drm/i915: Reject non-canonical rotations @ 2016-03-22 10:35 Matthew Auld 2016-03-22 10:43 ` Ville Syrjälä 2016-03-22 12:32 ` ✗ Fi.CI.BAT: warning for " Patchwork 0 siblings, 2 replies; 12+ messages in thread From: Matthew Auld @ 2016-03-22 10:35 UTC (permalink / raw) To: intel-gfx 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 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] drm/i915: Reject non-canonical rotations 2016-03-22 10:35 [PATCH] drm/i915: Reject non-canonical rotations Matthew Auld @ 2016-03-22 10:43 ` Ville Syrjälä 2016-03-22 10:48 ` Matthew Auld 2016-03-22 12:32 ` ✗ Fi.CI.BAT: warning for " Patchwork 1 sibling, 1 reply; 12+ messages in thread From: Ville Syrjälä @ 2016-03-22 10:43 UTC (permalink / raw) To: Matthew Auld; +Cc: intel-gfx 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] drm/i915: Reject non-canonical rotations 2016-03-22 10:43 ` Ville Syrjälä @ 2016-03-22 10:48 ` Matthew Auld 2016-03-22 10:59 ` Ville Syrjälä 0 siblings, 1 reply; 12+ messages in thread From: Matthew Auld @ 2016-03-22 10:48 UTC (permalink / raw) To: Ville Syrjälä; +Cc: intel-gfx 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] drm/i915: Reject non-canonical rotations 2016-03-22 10:48 ` Matthew Auld @ 2016-03-22 10:59 ` Ville Syrjälä 2016-03-22 11:36 ` Daniel Vetter 0 siblings, 1 reply; 12+ messages in thread From: Ville Syrjälä @ 2016-03-22 10:59 UTC (permalink / raw) To: Matthew Auld; +Cc: intel-gfx 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] drm/i915: Reject non-canonical rotations 2016-03-22 10:59 ` Ville Syrjälä @ 2016-03-22 11:36 ` Daniel Vetter 2016-03-22 14:14 ` Matthew Auld 0 siblings, 1 reply; 12+ messages in thread From: Daniel Vetter @ 2016-03-22 11:36 UTC (permalink / raw) To: Ville Syrjälä; +Cc: intel-gfx 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] drm/i915: Reject non-canonical rotations 2016-03-22 11:36 ` Daniel Vetter @ 2016-03-22 14:14 ` Matthew Auld 2016-03-23 8:58 ` Daniel Vetter 0 siblings, 1 reply; 12+ messages in thread From: Matthew Auld @ 2016-03-22 14:14 UTC (permalink / raw) To: Daniel Vetter, joonas.lahtinen; +Cc: intel-gfx 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] drm/i915: Reject non-canonical rotations 2016-03-22 14:14 ` Matthew Auld @ 2016-03-23 8:58 ` Daniel Vetter 2016-03-23 13:30 ` Joonas Lahtinen 0 siblings, 1 reply; 12+ messages in thread From: Daniel Vetter @ 2016-03-23 8:58 UTC (permalink / raw) To: Matthew Auld; +Cc: intel-gfx 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] drm/i915: Reject non-canonical rotations 2016-03-23 8:58 ` Daniel Vetter @ 2016-03-23 13:30 ` Joonas Lahtinen 2016-03-23 16:18 ` Daniel Vetter 0 siblings, 1 reply; 12+ messages in thread From: Joonas Lahtinen @ 2016-03-23 13:30 UTC (permalink / raw) To: Daniel Vetter, Matthew Auld; +Cc: intel-gfx 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] drm/i915: Reject non-canonical rotations 2016-03-23 13:30 ` Joonas Lahtinen @ 2016-03-23 16:18 ` Daniel Vetter 2016-03-23 16:24 ` Matthew Auld 2016-03-23 16:30 ` Ville Syrjälä 0 siblings, 2 replies; 12+ messages in thread From: Daniel Vetter @ 2016-03-23 16:18 UTC (permalink / raw) To: Joonas Lahtinen; +Cc: intel-gfx 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] drm/i915: Reject non-canonical rotations 2016-03-23 16:18 ` Daniel Vetter @ 2016-03-23 16:24 ` Matthew Auld 2016-03-23 16:30 ` Ville Syrjälä 1 sibling, 0 replies; 12+ messages in thread From: Matthew Auld @ 2016-03-23 16:24 UTC (permalink / raw) To: Daniel Vetter; +Cc: intel-gfx 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] drm/i915: Reject non-canonical rotations 2016-03-23 16:18 ` Daniel Vetter 2016-03-23 16:24 ` Matthew Auld @ 2016-03-23 16:30 ` Ville Syrjälä 1 sibling, 0 replies; 12+ messages in thread From: Ville Syrjälä @ 2016-03-23 16:30 UTC (permalink / raw) To: Daniel Vetter; +Cc: intel-gfx 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* ✗ Fi.CI.BAT: warning for drm/i915: Reject non-canonical rotations 2016-03-22 10:35 [PATCH] drm/i915: Reject non-canonical rotations Matthew Auld 2016-03-22 10:43 ` Ville Syrjälä @ 2016-03-22 12:32 ` Patchwork 1 sibling, 0 replies; 12+ messages in thread From: Patchwork @ 2016-03-22 12:32 UTC (permalink / raw) To: Matthew Auld; +Cc: intel-gfx == 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2016-03-23 16:30 UTC | newest] Thread overview: 12+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-03-22 10:35 [PATCH] drm/i915: Reject non-canonical rotations Matthew Auld 2016-03-22 10:43 ` Ville Syrjälä 2016-03-22 10:48 ` Matthew Auld 2016-03-22 10:59 ` Ville Syrjälä 2016-03-22 11:36 ` Daniel Vetter 2016-03-22 14:14 ` Matthew Auld 2016-03-23 8:58 ` Daniel Vetter 2016-03-23 13:30 ` Joonas Lahtinen 2016-03-23 16:18 ` Daniel Vetter 2016-03-23 16:24 ` Matthew Auld 2016-03-23 16:30 ` Ville Syrjälä 2016-03-22 12:32 ` ✗ Fi.CI.BAT: warning for " Patchwork
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).