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

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

On 07/19/2012 04:57 PM, 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@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

To make things more complicated add:

c) Line interleaved planes

[Y    Y    Y    Y    .... ]
[CBCR CBCR CBCR CBCR .... ]
[Y    Y    Y    Y    .... ]
[CBCR CBCR CBCR CBCR .... ]
[ ....................... ]

So maybe:

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

- Lars