[Intel-gfx] [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