Re: [PATCH 4/4] drm: Renesas SH Mobile DRM driver

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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