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

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

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.

-- 
Regards,

Laurent Pinchart