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