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

From: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
To: Daniel Vetter <daniel@ffwll.ch>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH] drm/i915: Reject non-canonical rotations
Date: Wed, 23 Mar 2016 18:30:05 +0200	[thread overview]
Message-ID: <20160323163005.GY4329@intel.com> (raw)
In-Reply-To: <20160323161808.GA2510@phenom.ffwll.local>

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