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

[Lars-Peter Clausen]

On 07/19/2012 03:33 PM, Laurent Pinchart wrote:
> Hi Lars,
> 
> On Thursday 19 July 2012 15:34:26 Lars-Peter Clausen wrote:
>> On 07/19/2012 02:46 PM, 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 at 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.
> 
> My understanding is that the offset is a linear offset from the start of the 
> buffer to allow X/Y panning. In that case the pitch is a frame buffer property 
> that is not be influenced by the offset at all.
> 

Hi,

I think panning is normally done by setting the x and y offset for the crtc.

But yes, you are right the offset might just be a linear offset to the start
of the actual data. I was just thinking about the case where the different
planes are interleaved. But for panning or non-interleaved planes
it obviously is different.

Though this leaves us with a problem. If the planes are interleaved and the
offset is included in the pitch your check may fail, even though the buffer
is large enough.

Maybe we need to handle both cases differently. If offset < pitch check for
obj->size < mode_cmd->height * mode_cmd->pitches[i] otherwise check for
obj->size < mode_cmd->height * mode_cmd->pitches[i] + mode_cmd->offsets[i]

But that doesn't quite work either if you have both interleaved planes and a
linear offset...

- Lars