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@xxxxxxxxxx>
>>>>>>>>>
>>>>>>>>> ---
>>>>>>>>> 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.
>
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
