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 <l...@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 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel