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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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


[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux