Re: [PATCH v3] DRM: Add DRM kms/fb cma helper
Hi Lars-Peter;
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
>
> ---
> 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
>
> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> index 41bbd95..e511c9a 100644
> --- a/drivers/gpu/drm/Kconfig
> +++ b/drivers/gpu/drm/Kconfig
> @@ -41,6 +41,16 @@ config DRM_GEM_CMA_HELPER
> help
> Choose this if you need the GEM CMA helper functions
>
> +config DRM_KMS_CMA_HELPER
> + tristate
> + select DRM_GEM_CMA_HELPER
> + select DRM_KMS_HELPER
> + select FB_SYS_FILLRECT
> + select FB_SYS_COPYAREA
> + select FB_SYS_IMAGEBLIT
> + help
> + Choose this if you need the KMS cma helper functions
> +
> config DRM_TDFX
> tristate "3dfx Banshee/Voodoo3+"
> depends on DRM && PCI
> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> index 6e9e948..5dcb1a5 100644
> --- a/drivers/gpu/drm/Makefile
> +++ b/drivers/gpu/drm/Makefile
> @@ -18,6 +18,7 @@ drm-$(CONFIG_COMPAT) += drm_ioc32.o
> drm-$(CONFIG_DRM_GEM_CMA_HELPER) += drm_gem_cma_helper.o
>
> drm_kms_helper-y := drm_fb_helper.o drm_crtc_helper.o drm_dp_i2c_helper.o
> +drm_kms_helper-$(CONFIG_DRM_KMS_CMA_HELPER) += drm_fb_cma_helper.o
This results in a built failure for me with
CONFIG_DRM=y
CONFIG_DRM_KMS_HELPER=m
# CONFIG_DRM_LOAD_EDID_FIRMWARE is not set
CONFIG_DRM_GEM_CMA_HELPER=m
CONFIG_DRM_KMS_CMA_HELPER=m
drm_fb_cma_helper.o isn't compiled at all. Can you reproduce the problem ?
> obj-$(CONFIG_DRM_KMS_HELPER) += drm_kms_helper.o
>
> 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 000..9042233
> --- /dev/null
> +++ b/drivers/gpu/drm/drm_fb_cma_helper.c
> @@ -0,0 +1,393 @@
> +/*
> + * drm kms/fb cma (contiguous memory allocator) helper functions
> + *
> + * Copyright (C) 2012 Analog Device Inc.
> + * Author: Lars-Peter Clausen
> + *
> + * Based on udl_fbdev.c
> + * Copyright (C) 2012 Red Hat
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version 2
> + * of the License, or (at your option) any later version.
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +struct drm_fb_cma {
> + struct drm_framebuffer fb;
> + struct drm_gem_cma_object *obj[4];
> +};
> +
> +struct drm_fbdev_cma {
> + struct drm_fb_helperfb_helper;
> + struct drm_fb_cma *fb;
> +};
> +
> +static inline struct drm_fbdev_cma *to_fbdev_cma(struct drm_fb_helper
> *helper) +{
> + return container_of(helper, struct drm_fbdev_cma, fb_helper);
> +}
> +
> +static inline struct drm_fb_cma *to_fb_cma(struct drm_framebuffer *fb)
> +{
> + return container_of(fb, struct drm_fb_cma, fb);
> +}
> +
> +static void drm_fb_cma_destroy(struct drm_framebuffer *fb)
> +{
> + struct drm_fb_cma *fb_cma = to_fb_cma(fb);
> + int i;
> +
> + for (i = 0; i < 4; i++) {
> + if (fb_cma->obj[i])
> +
> drm_gem_object_unreference_unlocked(&fb_cma->obj[i]->base);
> + }
> +
> + drm_framebuffer_cleanup(fb);
> + kfree(fb_cma);
> +}
> +
> +static int drm_fb_cma_create_handle(struct drm_framebuffer *fb,
> + struct drm_file *file_priv, unsigned int *handle)
> +{
> + struct drm_fb_cma *fb_cma = to_fb_cma(fb);
> +
> + return drm_gem_handle_create(file_priv,
> + &fb_cma->obj[0]->base, handle);
> +}
> +
> +static struct drm_framebuffer_funcs drm_fb_cma_funcs = {
> + .destroy= drm_fb_cma_destroy,
> + .create_handle = drm_fb_cma_create_handle,
> +};
> +
> +static struct drm_fb_cma *drm_fb_cma_alloc(struct drm_device *dev,
> + struct drm_mode_fb_cmd2 *mode_c
Re: [PATCH v3] DRM: Add DRM kms/fb cma helper
On 07/19/2012 05:46 PM, Laurent Pinchart wrote:
> 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
>>
>> ---
>> 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 000..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] +
>>
Re: [PATCH v3] DRM: Add DRM kms/fb cma helper
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
>
> ---
> 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 000..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]
> >
Re: [PATCH v3] DRM: Add DRM kms/fb cma helper
On 07/19/2012 05:01 PM, Laurent Pinchart wrote:
> 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
>
> ---
> 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 000..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...
>
Re: [PATCH v3] DRM: Add DRM kms/fb cma helper
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
---
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 000..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
Re: [PATCH v3] DRM: Add DRM kms/fb cma helper
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
> >>>
> >>> ---
> >>> 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 000..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 findin
Re: [PATCH v3] DRM: Add DRM kms/fb cma helper
On 07/19/2012 04:57 PM, Lars-Peter Clausen wrote: > And well something similar for horizontal subsampling. s/horizontal/vertical/ ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v3] DRM: Add DRM kms/fb cma helper
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
>>>
>>> ---
>>> 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 000..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
Re: [PATCH v3] DRM: Add DRM kms/fb cma helper
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
> >
> > ---
> > 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 000..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 beg
Re: [PATCH v3] DRM: Add DRM kms/fb cma helper
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
>
> ---
> 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 000..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@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v3] DRM: Add DRM kms/fb cma helper
On 07/19/2012 03:33 PM, Laurent Pinchart wrote:
> Hi Lars,
>
> 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
---
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 000..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...
- Lars
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v3] DRM: Add DRM kms/fb cma helper
Hi Lars,
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
> >>
> >> ---
> >> 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 000..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.
--
Regards,
Laurent Pinchart
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v3] DRM: Add DRM kms/fb cma helper
On Thursday 19 July 2012 14:46:38 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
> >
> > ---
> > 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 000..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])
>
> ?
And, for multiplanar formats, we need to divide height by the plane vertical
subsampling factor. What about something like this ?
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++) {
unsigned int min_size;
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;
}
min_size = mode_cmd->pitches[i] * mode_cmd->height;
if (i)
min_size /=
drm_format_vert_chroma_subsampling(mode_cmd->pixel_format);
min_size += mode_cmd->offsets[i];
if (obj->size < min_size) {
drm_gem_object_unreference_unlocked(obj);
ret = -EINVAL;
goto err_gem_object_unreference;
}
objs[i] = to_drm_gem_cma_obj(obj);
}
On a totally unrelated note, I'm wondering whether we should replace the
various DRM format information calls (drm_format_num_planes,
drm_fb_get_bpp_depth, ...) by a single drm_get_format_info() function that
would return a const pointer to format information. We could keep the existing
API as static inline functions. This would save CPU time when we need to
repeatedly access various information about a single format.
> > + drm_gem_object_unreference_unlocked(obj);
> > + ret = -EINVAL;
> > + goto err_gem_object_unreference;
> > + }
> > + objs[i] = to_drm_gem_cma_obj(obj);
> > + }
> > +
> > + fb_cma = drm_fb_cma_alloc(dev, mode_cmd, objs, i);
> > + if (IS_ERR(fb_cma)) {
> > + ret = PTR_ERR(fb_cma);
> > + goto err_gem_object_unreference;
> > + }
> > +
> > + return &fb_cma->fb;
> > +
> > +err_gem_object_unreference:
> > + for (i--; i >= 0; i--)
> > + drm_gem_object_unreference_unlocked(&objs[i]->base);
> > + return ERR_PTR(ret);
> > +}
> > +EXPORT_SYMBOL_GPL(drm_fb_cma_create);
--
Regards,
Laurent Pinchart
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/m
Re: [PATCH v3] DRM: Add DRM kms/fb cma helper
On 07/19/2012 02:46 PM, Laurent Pinchart wrote:
> Hi Lars,
>
> Thank you for the patch.
>
> 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
>>
>> ---
>> 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 000..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.
Thanks,
- Lars
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v3] DRM: Add DRM kms/fb cma helper
Hi Lars,
Thank you for the patch.
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
>
> ---
> 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 000..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])
?
> + drm_gem_object_unreference_unlocked(obj);
> + ret = -EINVAL;
> + goto err_gem_object_unreference;
> + }
> + objs[i] = to_drm_gem_cma_obj(obj);
> + }
> +
> + fb_cma = drm_fb_cma_alloc(dev, mode_cmd, objs, i);
> + if (IS_ERR(fb_cma)) {
> + ret = PTR_ERR(fb_cma);
> + goto err_gem_object_unreference;
> + }
> +
> + return &fb_cma->fb;
> +
> +err_gem_object_unreference:
> + for (i--; i >= 0; i--)
> + drm_gem_object_unreference_unlocked(&objs[i]->base);
> + return ERR_PTR(ret);
> +}
> +EXPORT_SYMBOL_GPL(drm_fb_cma_create);
--
Regards,
Laurent Pinchart
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v3] DRM: Add DRM kms/fb cma helper
On Mon, Jul 02, 2012 at 04:37:47PM +0200, 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
>
> ---
> 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
Tested-by: Sascha Hauer
Would be good if this could go into the next merge window.
Sascha
>
> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> index 41bbd95..e511c9a 100644
> --- a/drivers/gpu/drm/Kconfig
> +++ b/drivers/gpu/drm/Kconfig
> @@ -41,6 +41,16 @@ config DRM_GEM_CMA_HELPER
> help
> Choose this if you need the GEM CMA helper functions
>
> +config DRM_KMS_CMA_HELPER
> + tristate
> + select DRM_GEM_CMA_HELPER
> + select DRM_KMS_HELPER
> + select FB_SYS_FILLRECT
> + select FB_SYS_COPYAREA
> + select FB_SYS_IMAGEBLIT
> + help
> + Choose this if you need the KMS cma helper functions
> +
> config DRM_TDFX
> tristate "3dfx Banshee/Voodoo3+"
> depends on DRM && PCI
> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> index 6e9e948..5dcb1a5 100644
> --- a/drivers/gpu/drm/Makefile
> +++ b/drivers/gpu/drm/Makefile
> @@ -18,6 +18,7 @@ drm-$(CONFIG_COMPAT) += drm_ioc32.o
> drm-$(CONFIG_DRM_GEM_CMA_HELPER) += drm_gem_cma_helper.o
>
> drm_kms_helper-y := drm_fb_helper.o drm_crtc_helper.o drm_dp_i2c_helper.o
> +drm_kms_helper-$(CONFIG_DRM_KMS_CMA_HELPER) += drm_fb_cma_helper.o
>
> obj-$(CONFIG_DRM_KMS_HELPER) += drm_kms_helper.o
>
> 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 000..9042233
> --- /dev/null
> +++ b/drivers/gpu/drm/drm_fb_cma_helper.c
> @@ -0,0 +1,393 @@
> +/*
> + * drm kms/fb cma (contiguous memory allocator) helper functions
> + *
> + * Copyright (C) 2012 Analog Device Inc.
> + * Author: Lars-Peter Clausen
> + *
> + * Based on udl_fbdev.c
> + * Copyright (C) 2012 Red Hat
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version 2
> + * of the License, or (at your option) any later version.
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +struct drm_fb_cma {
> + struct drm_framebuffer fb;
> + struct drm_gem_cma_object *obj[4];
> +};
> +
> +struct drm_fbdev_cma {
> + struct drm_fb_helperfb_helper;
> + struct drm_fb_cma *fb;
> +};
> +
> +static inline struct drm_fbdev_cma *to_fbdev_cma(struct drm_fb_helper
> *helper)
> +{
> + return container_of(helper, struct drm_fbdev_cma, fb_helper);
> +}
> +
> +static inline struct drm_fb_cma *to_fb_cma(struct drm_framebuffer *fb)
> +{
> + return container_of(fb, struct drm_fb_cma, fb);
> +}
> +
> +static void drm_fb_cma_destroy(struct drm_framebuffer *fb)
> +{
> + struct drm_fb_cma *fb_cma = to_fb_cma(fb);
> + int i;
> +
> + for (i = 0; i < 4; i++) {
> + if (fb_cma->obj[i])
> +
> drm_gem_object_unreference_unlocked(&fb_cma->obj[i]->base);
> + }
> +
> + drm_framebuffer_cleanup(fb);
> + kfree(fb_cma);
> +}
> +
> +static int drm_fb_cma_create_handle(struct drm_framebuffer *fb,
> + struct drm_file *file_priv, unsigned int *handle)
> +{
> + struct drm_fb_cma *fb_cma = to_fb_cma(fb);
> +
> + return drm_gem_handle_create(file_priv,
> + &fb_cma->obj[0]->base, handle);
> +}
> +
> +static struct drm_framebuffer_funcs drm_fb_cma_funcs = {
> + .destroy= drm_fb_cma_destroy,
> + .create_handle = drm_fb_cma_create_handle,
> +};
> +
> +static struct drm_fb_cma *drm_fb_cma_alloc(struct drm_device *dev,
> + struct drm_mode_fb_cmd2 *mode_cmd, struct drm_gem_cma_object **obj,
> + unsigned int num_planes)
> +{
> + struct drm_fb_cma *fb_cma;
> + int ret;
> + int i;
> +
> +
