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

[Lars-Peter Clausen]

On 07/19/2012 04:41 PM, Laurent Pinchart wrote:
> On Thursday 19 July 2012 16:02:41 Laurent Pinchart wrote:
>> On Thursday 19 July 2012 15:55:56 Lars-Peter Clausen wrote:
>>> On 07/19/2012 03:33 PM, Laurent Pinchart wrote:
>>>> 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...
>>
>> What about first finding out what those offsets are supposed to be used for
>> ?
>>
>> Ville, git blame points to you as the author of the offsets field :-) Could
>> you please comment on this ?
> 
> My bad, I was looking at drm_framebuffer and not drm_mode_fb_cmd2.
> 
> Offset really looks like it is a linear offset from the beginning of the 
> buffer. This allows placing several planes at different locations in a single 
> buffer. I think we need to add mode_cmd->offsets[i] unconditionally when 
> checking the size.
> 

There are two cases to consider interleaved and non-interleaved planes in a
single buffer.

a) Separate planes
[Y Y Y Y ....]
[Y Y Y Y ....]
[ .......... ]
[Y Y Y Y ....]
[CBCR CBCR CBCR CBCR .... ]
[CBCR CBCR CBCR CBCR .... ]
[ ....................... ]
[CBCR CBCR CBCR CBCR .... ]

Plane 0:
bpp = 1
offset = 0

Plane 1:
bpp = 1
offset = plane0_size


b) Interleaved planes
[Y CBCR Y CBCR Y CBCR ....]
[Y CBCR Y CBCR Y CBCR ....]
[ ....................... ]
[Y CBCR Y CBCR Y CBCR ....]

Plane 0:
bbp = 2
offset = 0

Plane 1:
bbp = 2
offset = 1


For case b) pitch * height + offset will point after the end of the buffer
by one pixel, which is the size of the Y component.

I think we need something like:

min_size = height * pitch;
min_size += offset - (bpp - drm_format_plane_cpp())

(bpp - drm_format_plane_cpp() is, so it will be zero for the separate planes
case,


And well something similar for horizontal subsampling.

- Lars