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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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@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

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) ?

> 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.

-- 
Regards,

Laurent Pinchart