Hi Ville,
Thank you for the review.
On Wednesday 30 May 2012 17:16:46 Ville Syrjälä 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 = shmob_drm_format_info(mode_cmd->pixel_format);
> > + if (format == NULL) {
> > + dev_dbg(dev->dev, "unsupported pixel format %08x\n",
> > + mode_cmd->pixel_format);
> > + return ERR_PTR(-EINVAL);
> > + }
> > +
> > + sfb = kzalloc(sizeof(*fb), GFP_KERNEL);
> > + if (sfb == NULL)
> > + return ERR_PTR(-ENOMEM);
> > +
> > + sfb->format = format;
> > + fb = &sfb->base;
> > +
> > + for (i = 0; i < (format->yuv ? 2 : 1); ++i) {
> > + obj = drm_gem_object_lookup(dev, file_priv,
> > + mode_cmd->handles[i]);
> > + if (obj == NULL) {
> > + dev_dbg(dev->dev, "GEM object %u not found\n",
> > + mode_cmd->handles[i]);
> > + ret = -ENOENT;
> > + goto error;
> > + }
> > + sfb->sobj[i] = 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] == pitches[0]*chroma_cpp. You should reject other values here.
> Also it's not clear from the code if there are other stride limitations that
> 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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
