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@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 .... ] [ ....................... ]
So maybe:
min_size = height * pitch; min_size -= bpp - drm_format_plane_cpp(); min_size -= pitch - width * bpp; min_size += offset;
- Lars