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

[Lars-Peter Clausen <lars@metafoo.de>]

On 07/19/2012 02:46 PM, Laurent Pinchart wrote:
> Hi Lars,
> 
> Thank you for the patch.
> 
> 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])
> 
> ?

That's actually a good question. I'd expect the offset to be included in the
pitch.

If you access pixels like mem[offset + x * bpp + y * pitch] then pitch has
to be greater equal to offset + max_x * bpp, otherwise you'd have
overlapping lines.

Thanks,
- Lars