On 07/19/2012 05:46 PM, Laurent Pinchart wrote:
> Hi Lars,
>
> On Thursday 19 July 2012 17:12:28 Lars-Peter Clausen wrote:
>> 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 .... ]
>> [ ....................... ]
>
> This is a valid use case, and I see the issue. We need to take into account
> the fact that the last line of each plane doesn't include padding.
>
>> So maybe:
>>
>> min_size = height * pitch;
>> min_size -= bpp - drm_format_plane_cpp();
>> min_size -= pitch - width * bpp;
>> min_size += offset;
>
> What about
>
> hsub = drm_format_horz_chroma_subsampling(mode_cmd->pixel_format);
> vsub = drm_format_vert_chroma_subsampling(mode_cmd->pixel_format);
>
> for ( ... ) {
> ...
> width = mode_cmd->width / (i ? hsub : 1);
> height = mode_cmd->height / (i ? vsub : 1);
>
> min_size = (height - 1) * mode_cmd->pitches[i]
> + width * drm_format_plane_cpp(mode_cmd->pixel_format, i)
> + mode_cmd->offsets[i];
>
> ...
> }
>
Yes that should work. I'll prepare a new version.
Thanks,
- Lars
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
