From mboxrd@z Thu Jan 1 00:00:00 1970 From: Laurent Pinchart Subject: Re: [PATCH 4/4] drm: Renesas SH Mobile DRM driver Date: Wed, 27 Jun 2012 14:40:37 +0200 Message-ID: <13412039.USzRhKQPyi@avalon> References: <1338381179-13290-1-git-send-email-laurent.pinchart@ideasonboard.com> <1338381179-13290-5-git-send-email-laurent.pinchart@ideasonboard.com> <20120530141646.GT13065@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Received: from perceval.ideasonboard.com (perceval.ideasonboard.com [95.142.166.194]) by gabe.freedesktop.org (Postfix) with ESMTP id 395C59E9C3 for ; Wed, 27 Jun 2012 05:40:34 -0700 (PDT) In-Reply-To: <20120530141646.GT13065@intel.com> 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: Ville =?ISO-8859-1?Q?Syrj=E4l=E4?= Cc: dri-devel@lists.freedesktop.org List-Id: dri-devel@lists.freedesktop.org Hi Ville, Thank you for the review. On Wednesday 30 May 2012 17:16:46 Ville Syrj=E4l=E4 wrote: > On Wed, May 30, 2012 at 02:32:59PM +0200, Laurent Pinchart wrote: > > +static struct drm_framebuffer * > > +shmob_drm_fb_create(struct drm_device *dev, struct drm_file *file_priv, > > + struct drm_mode_fb_cmd2 *mode_cmd) > > +{ > > + const struct shmob_drm_format_info *format; > > + struct shmob_drm_framebuffer *sfb; > > + struct drm_framebuffer *fb; > > + struct drm_gem_object *obj; > > + unsigned int i; > > + int ret; > > + > > + format =3D shmob_drm_format_info(mode_cmd->pixel_format); > > + if (format =3D=3D NULL) { > > + dev_dbg(dev->dev, "unsupported pixel format %08x\n", > > + mode_cmd->pixel_format); > > + return ERR_PTR(-EINVAL); > > + } > > + > > + sfb =3D kzalloc(sizeof(*fb), GFP_KERNEL); > > + if (sfb =3D=3D NULL) > > + return ERR_PTR(-ENOMEM); > > + > > + sfb->format =3D format; > > + fb =3D &sfb->base; > > + > > + for (i =3D 0; i < (format->yuv ? 2 : 1); ++i) { > > + obj =3D drm_gem_object_lookup(dev, file_priv, > > + mode_cmd->handles[i]); > > + if (obj =3D=3D NULL) { > > + dev_dbg(dev->dev, "GEM object %u not found\n", > > + mode_cmd->handles[i]); > > + ret =3D -ENOENT; > > + goto error; > > + } > > + sfb->sobj[i] =3D to_shmob_gem_object(obj); > > + } > = > offsets[] not checked, nor is it handled in the code that calculates the > final offsets. > = > Based on the rest of the code, it seems that the hardware assumes > pitches[1] =3D=3D pitches[0]*chroma_cpp. You should reject other values h= ere. > Also it's not clear from the code if there are other stride limitations t= hat > would need checking. > = > Also there are no checks to make sure the fb fits inside the gem object. = At > one point I tried to cook up a generic function to help with such checks.= I > probably need to revisit that issue and try to make something that'd work > for tiled formats as well. You're right. I now use the generic drm_fb_cma_create() function from Lars- Peter "DRM: Add DRM kms/fb cma helper" patch, so the above code is gone. I'= ll = make sure your comments get addressed in the patch. -- = Regards, Laurent Pinchart