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

[Matt Roper <matthew.d.roper@intel.com>]

On Fri, May 23, 2014 at 12:51:29AM +0200, Daniel Vetter wrote:
> 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

When we switch to atomic modeset in i-g-t, everything will be coming in
via the propertyset interface, right?  Since the range limiting check is
in the setplane ioctl code now, I'd expect us to bypass it completely.


Matt

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795