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

[Lars-Peter Clausen]

On 07/19/2012 05:01 PM, Laurent Pinchart wrote:
> Hi Lars,
> 
> On Thursday 19 July 2012 16:57:15 Lars-Peter Clausen wrote:
>> 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
> 
> I might be missing something, but isn't this supposed to be handled with a 
> single plane using packed YUV formats (DRM_FORMAT_YUYV & co) ?
> 

Maybe, but at least line interleaved formats seem to be reality, see:
http://lists.freedesktop.org/archives/dri-devel/2012-April/021044.html

>> 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.
>