| Message ID | 1458642941-8092-1-git-send-email-matthew.auld@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
As in the drm core? Not as far as I could tell...
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 ;)
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
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
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
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
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
Okay, I'll rework the patch to use drm_rotation_simplify instead.
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.
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)) {
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(+)