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

From: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
To: Daniel Vetter <daniel@ffwll.ch>,
	Matthew Auld <matthew.william.auld@gmail.com>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH] drm/i915: Reject non-canonical rotations
Date: Wed, 23 Mar 2016 15:30:48 +0200	[thread overview]
Message-ID: <1458739848.31507.4.camel@linux.intel.com> (raw)
In-Reply-To: <20160323085831.GK28483@phenom.ffwll.local>

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