Hi Lars,
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 ?
--
Regards,
Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
