Re: [PATCH v3] DRM: Add DRM kms/fb cma helper

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

On Thursday 19 July 2012 14:46:38 Laurent Pinchart wrote:
> On Monday 02 July 2012 16:37:47 Lars-Peter Clausen wrote:
> > This patchset introduces a set of helper function for implementing the KMS
> > framebuffer layer for drivers which use the drm gem CMA helper function.
> > 
> > Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
> > 
> > ---
> > Note: This patch depends on Sascha's "DRM: add drm gem CMA helper" patch
> > 
> > Changes since v2:
> > 	* Adapt to changes in the GEM CMA helper
> > 	* Add basic buffer size checking in drm_fb_cma_create
> > 
> > Changes since v1:
> > 	* Some spelling fixes
> > 	* Add missing kfree in drm_fb_cma_alloc error path
> > 	* Add multi-plane support
> > 
> > ---
> > 
> >  drivers/gpu/drm/Kconfig             |   10 +
> >  drivers/gpu/drm/Makefile            |    1 +
> >  drivers/gpu/drm/drm_fb_cma_helper.c |  393
> >  ++++++++++++++++++++++++++++++++
> >  include/drm/drm_fb_cma_helper.h     |   27 +++
> >  4 files changed, 431 insertions(+)
> >  create mode 100644 drivers/gpu/drm/drm_fb_cma_helper.c
> >  create mode 100644 include/drm/drm_fb_cma_helper.h
> 
> [snip]
> 
> > diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c
> > b/drivers/gpu/drm/drm_fb_cma_helper.c new file mode 100644
> > index 0000000..9042233
> > --- /dev/null
> > +++ b/drivers/gpu/drm/drm_fb_cma_helper.c
> 
> [snip]
> 
> > +/**
> > + * drm_fb_cma_create() - (struct drm_mode_config_funcs *)->fb_create
> > callback function
> > + *
> > + * If your hardware has special alignment or pitch requirements these
> > should be
> > + * checked before calling this function.
> > + */
> > +struct drm_framebuffer *drm_fb_cma_create(struct drm_device *dev,
> > +	struct drm_file *file_priv, struct drm_mode_fb_cmd2 *mode_cmd)
> > +{
> > +	struct drm_fb_cma *fb_cma;
> > +	struct drm_gem_cma_object *objs[4];
> > +	struct drm_gem_object *obj;
> > +	int ret;
> > +	int i;
> > +
> > +	for (i = 0; i < drm_format_num_planes(mode_cmd->pixel_format); i++) {
> > +		obj = drm_gem_object_lookup(dev, file_priv, mode_cmd->handles[i]);
> > +		if (!obj) {
> > +			dev_err(dev->dev, "Failed to lookup GEM object\n");
> > +			ret = -ENXIO;
> > +			goto err_gem_object_unreference;
> > +		}
> > +
> > +		if (obj->size < mode_cmd->height * mode_cmd->pitches[i]) {
> 
> Shouldn't this be
> 
> if (obj->size < mode_cmd->height * mode_cmd->pitches[i]
>               + mode_cmd->offsets[i])
> 
> ?

And, for multiplanar formats, we need to divide height by the plane vertical
subsampling factor. What about something like this ?

        struct drm_fb_cma *fb_cma;
        struct drm_gem_cma_object *objs[4];
        struct drm_gem_object *obj;
        int ret;
        int i;

        for (i = 0; i < drm_format_num_planes(mode_cmd->pixel_format); i++) {
                unsigned int min_size;

                obj = drm_gem_object_lookup(dev, file_priv, mode_cmd->handles[i]);
                if (!obj) {
                        dev_err(dev->dev, "Failed to lookup GEM object\n");
                        ret = -ENXIO;
                        goto err_gem_object_unreference;
                }

                min_size = mode_cmd->pitches[i] * mode_cmd->height;
                if (i)
                        min_size /= drm_format_vert_chroma_subsampling(mode_cmd->pixel_format);
                min_size += mode_cmd->offsets[i];

                if (obj->size < min_size) {
                        drm_gem_object_unreference_unlocked(obj);
                        ret = -EINVAL;
                        goto err_gem_object_unreference;
                }
                objs[i] = to_drm_gem_cma_obj(obj);
        }

On a totally unrelated note, I'm wondering whether we should replace the
various DRM format information calls (drm_format_num_planes,
drm_fb_get_bpp_depth, ...) by a single drm_get_format_info() function that
would return a const pointer to format information. We could keep the existing
API as static inline functions. This would save CPU time when we need to
repeatedly access various information about a single format.

> > +			drm_gem_object_unreference_unlocked(obj);
> > +			ret = -EINVAL;
> > +			goto err_gem_object_unreference;
> > +		}
> > +		objs[i] = to_drm_gem_cma_obj(obj);
> > +	}
> > +
> > +	fb_cma = drm_fb_cma_alloc(dev, mode_cmd, objs, i);
> > +	if (IS_ERR(fb_cma)) {
> > +		ret = PTR_ERR(fb_cma);
> > +		goto err_gem_object_unreference;
> > +	}
> > +
> > +	return &fb_cma->fb;
> > +
> > +err_gem_object_unreference:
> > +	for (i--; i >= 0; i--)
> > +		drm_gem_object_unreference_unlocked(&objs[i]->base);
> > +	return ERR_PTR(ret);
> > +}
> > +EXPORT_SYMBOL_GPL(drm_fb_cma_create);

-- 
Regards,

Laurent Pinchart