Re: [PATCH 2/6] drm: Refactor setplane to allow internal use (v3)

[Daniel Vetter <daniel@ffwll.ch>]

On Thu, May 22, 2014 at 03:48:16PM -0700, Matt Roper wrote:
> On Fri, May 23, 2014 at 12:37:52AM +0200, Daniel Vetter wrote:
> > On Thu, May 22, 2014 at 6:33 PM, Matt Roper <matthew.d.roper@intel.com> wrote:
> > > v3:
> > >  - Move integer overflow checking from setplane_internal to setplane
> > >    ioctl.  The upcoming legacy cursor support via universal planes needs
> > >    to maintain current cursor ioctl semantics and not return error for
> > >    these extreme values (found via intel-gpu-tools kms_cursor_crc test).
> > 
> > Imo that's just the test being silly. I think it's valid to reject
> > such nonsense with -EINVAL and limit the random-walk range of our
> > testcase a bit. The igt testcases are just guidelines for what our ABI
> > is, but not the hard rules. And clarifying the ABI in such cases is
> > imo the right approach.
> > 
> > That random-walk testcase just was meant to test correctly clamping.
> > E.g. on hardware with just 12bit for the cursor placement we'd wrap
> > around and show a cursor again that should be off. So as long as you
> > still test that you can restrict the range of the test a bit.
> > -Daniel
> 
> I was a bit torn on this.  Since we accept offscreen plane positions,
> I'm not sure it makes sense to really treat "offscreen" differently than
> "really, really offscreen."  The legacy cursor ioctls have never cared
> about this in the past so I didn't want to add in artificial range
> limiting just because the setplane ioctl has had it for a while.
> 
> There's actually an i-g-t test that specifically tries to program
> specifically at INT_MAX (not just the random walk test that might get
> lucky and go out of bounds), so I figured the best course was just to
> preserve the current behavior; I can't really see any downsides to not
> range-limiting the cursors.

Ok, if you think this won't create an unnecessary fuss in the code then
I'm ok with this, too. After all most hw has limited offset fields, so
drivers must clamp anyway. So dunno really how much sense that limit check
makes in set_plane. But we have it, so doesn't hurt too much really.

A different consideration though is that with the igt_kms library the plan
is to just switch to atomic modeset eventually, which also means using
universal planes for cursors and primary planes. Which means as soon as we
do that we'll likely hit that restriction again.

Not sure what to do now here.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch