From mboxrd@z Thu Jan 1 00:00:00 1970 From: Lars-Peter Clausen Subject: Re: [PATCH v3] DRM: Add DRM kms/fb cma helper Date: Thu, 19 Jul 2012 18:01:53 +0200 Message-ID: <50082F71.7020800@metafoo.de> References: <1341239867-10384-1-git-send-email-lars@metafoo.de> <5008204B.7070205@metafoo.de> <500823DC.90104@metafoo.de> <2632033.zls6y4siQz@avalon> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from smtp-out-169.synserver.de (smtp-out-169.synserver.de [212.40.185.169]) by gabe.freedesktop.org (Postfix) with ESMTP id 58B859E764 for ; Thu, 19 Jul 2012 08:57:50 -0700 (PDT) In-Reply-To: <2632033.zls6y4siQz@avalon> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: dri-devel-bounces+sf-dri-devel=m.gmane.org@lists.freedesktop.org Errors-To: dri-devel-bounces+sf-dri-devel=m.gmane.org@lists.freedesktop.org To: Laurent Pinchart Cc: dri-devel@lists.freedesktop.org List-Id: dri-devel@lists.freedesktop.org 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 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]; > > ... > } > Yes that should work. I'll prepare a new version. Thanks, - Lars