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