From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Subject: Re: [PATCH 4/7] drm: plane: Check source coordinates Date: Fri, 11 Nov 2011 19:18:52 +0200 Message-ID: <20111111171851.GE3477@intel.com> References: <1321027447-17921-1-git-send-email-ville.syrjala@linux.intel.com> <1321027447-17921-5-git-send-email-ville.syrjala@linux.intel.com> <20111111082418.5dd4c93a@jbarnes-desktop> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by gabe.freedesktop.org (Postfix) with ESMTP id 60D289E7FB for ; Fri, 11 Nov 2011 09:16:51 -0800 (PST) Content-Disposition: inline In-Reply-To: <20111111082418.5dd4c93a@jbarnes-desktop> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: dri-devel-bounces+sf-dri-devel=m.gmane.org@lists.freedesktop.org Errors-To: dri-devel-bounces+sf-dri-devel=m.gmane.org@lists.freedesktop.org To: Jesse Barnes Cc: dri-devel@lists.freedesktop.org List-Id: dri-devel@lists.freedesktop.org On Fri, Nov 11, 2011 at 08:24:18AM -0800, Jesse Barnes wrote: > On Fri, 11 Nov 2011 18:04:04 +0200 > ville.syrjala@linux.intel.com wrote: > = > > From: Ville Syrj=E4l=E4 > > = > > Make sure the source coordinates stay within the buffer. > > = > > Signed-off-by: Ville Syrj=E4l=E4 > > --- > > drivers/gpu/drm/drm_crtc.c | 23 +++++++++++++++++++++++ > > 1 files changed, 23 insertions(+), 0 deletions(-) > > = > > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c > > index 70f5747..098cc50 100644 > > --- a/drivers/gpu/drm/drm_crtc.c > > +++ b/drivers/gpu/drm/drm_crtc.c > > @@ -1654,6 +1654,7 @@ int drm_mode_setplane(struct drm_device *dev, voi= d *data, > > struct drm_crtc *crtc; > > struct drm_framebuffer *fb; > > int ret =3D 0; > > + unsigned int fb_width, fb_height; > > = > > if (!drm_core_check_feature(dev, DRIVER_MODESET)) > > return -EINVAL; > > @@ -1702,6 +1703,28 @@ int drm_mode_setplane(struct drm_device *dev, vo= id *data, > > } > > fb =3D obj_to_fb(obj); > > = > > + fb_width =3D fb->width << 16; > > + fb_height =3D fb->height << 16; > > + > > + /* Make sure source coordinates are inside the fb. */ > > + if (plane_req->src_w > fb_width || > > + plane_req->src_x > fb_width - plane_req->src_w || > > + plane_req->src_h > fb_height || > > + plane_req->src_y > fb_height - plane_req->src_h) { > > + DRM_DEBUG_KMS("Invalid source coordinates " > > + "%01u.%06ux%01u.%06u+%01u.%06u+%01u.%06u\n", > > + plane_req->src_w >> 16, > > + ((plane_req->src_w & 0xffff) * 15625) >> 10, > > + plane_req->src_h >> 16, > > + ((plane_req->src_h & 0xffff) * 15625) >> 10, > > + plane_req->src_x >> 16, > > + ((plane_req->src_x & 0xffff) * 15625) >> 10, > > + plane_req->src_y >> 16, > > + ((plane_req->src_y & 0xffff) * 15625) >> 10); > > + ret =3D -EINVAL; > > + goto out; > > + } > > + > > ret =3D plane->funcs->update_plane(plane, crtc, fb, > > plane_req->crtc_x, plane_req->crtc_y, > > plane_req->crtc_w, plane_req->crtc_h, > = > Good sanity check (saves the drivers from having to do it), but I > wonder if we can use a better return value like ENOSPC or something to > make it easier for userspace to figure out. Yeah, getting EINVAL for every kind of failure is rather annoying. The only issue I have with ENOSPC is the strerror() output. It doesn't exactly fit this use case. But if there's nothing better I'm OK with ENOSPC. -- = Ville Syrj=E4l=E4 Intel OTC