From mboxrd@z Thu Jan 1 00:00:00 1970 From: Matt Roper Subject: Re: [PATCH 2/6] drm: Refactor setplane to allow internal use (v3) Date: Thu, 22 May 2014 15:57:31 -0700 Message-ID: <20140522225731.GS31484@intel.com> References: <1400543922-11078-3-git-send-email-matthew.d.roper@intel.com> <1400776435-19673-1-git-send-email-matthew.d.roper@intel.com> <20140522224816.GR31484@intel.com> <20140522225129.GQ14357@phenom.ffwll.local> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by gabe.freedesktop.org (Postfix) with ESMTP id C7E9B6ECE3 for ; Thu, 22 May 2014 15:56:23 -0700 (PDT) Content-Disposition: inline In-Reply-To: <20140522225129.GQ14357@phenom.ffwll.local> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Daniel Vetter Cc: dri-devel List-Id: dri-devel@lists.freedesktop.org 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 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