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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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.

-- 
Ville Syrjälä
Intel OTC