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

[Laurent Pinchart]

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 at 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
> 
> 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];

		...
	}

-- 
Regards,

Laurent Pinchart