From mboxrd@z Thu Jan  1 00:00:00 1970
From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Subject: Re: [PATCH v2] DRM: add drm gem cma helper
Date: Tue, 05 Jun 2012 10:05:52 +0200
Message-ID: <3660985.F0c9Kc1UOR@avalon>
References: <1338451734-20232-1-git-send-email-s.hauer@pengutronix.de>
	<2093871.VgT5F32Ubz@avalon> <20120605074948.GO30400@pengutronix.de>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <dri-devel-bounces+sf-dri-devel=m.gmane.org@lists.freedesktop.org>
Received: from perceval.ideasonboard.com (perceval.ideasonboard.com
	[95.142.166.194])
	by gabe.freedesktop.org (Postfix) with ESMTP id 85E099ECDD
	for <dri-devel@lists.freedesktop.org>;
	Tue,  5 Jun 2012 01:05:55 -0700 (PDT)
In-Reply-To: <20120605074948.GO30400@pengutronix.de>
List-Unsubscribe: <http://lists.freedesktop.org/mailman/options/dri-devel>,
	<mailto:dri-devel-request@lists.freedesktop.org?subject=unsubscribe>
List-Archive: <http://lists.freedesktop.org/archives/dri-devel>
List-Post: <mailto:dri-devel@lists.freedesktop.org>
List-Help: <mailto:dri-devel-request@lists.freedesktop.org?subject=help>
List-Subscribe: <http://lists.freedesktop.org/mailman/listinfo/dri-devel>,
	<mailto:dri-devel-request@lists.freedesktop.org?subject=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: Sascha Hauer <s.hauer@pengutronix.de>
Cc: dri-devel@lists.freedesktop.org
List-Id: dri-devel@lists.freedesktop.org

Hi Sascha,

On Tuesday 05 June 2012 09:49:48 Sascha Hauer wrote:
> On Thu, May 31, 2012 at 11:36:15AM +0200, Laurent Pinchart wrote:
> > Hi Sascha,
> > 
> > > +	depends on DRM
> > > +	help
> > > +	  Choose this if you need the GEM cma helper functions
> > 
> > I would put CMA in uppercase, but that's just nitpicking.
> > 
> > BTW this helper is not strictly dedicated to CMA. It uses the DMA API to
> > allocate memory, without caring about the underlying allocator.
> 
> Yes, I know. It's just that 'CMA' is short and expresses very much what
> I mean. I first had 'dma_alloc' instead, but this makes the function
> names quite long.
>
> > > +
> > > +/*
> > > + * drm_gem_cma_dumb_create - (struct drm_driver)->dumb_create callback
> > > + * function
> > > + *
> > > + * This aligns the pitch and size arguments to the minimum required.
> > > wrap
> > > + * this into your own function if you need bigger alignment.
> > > + */
> > > +int drm_gem_cma_dumb_create(struct drm_file *file_priv,
> > > +		struct drm_device *dev, struct drm_mode_create_dumb *args)
> > > +{
> > > +	struct drm_gem_cma_object *cma_obj;
> > > +
> > > +	if (args->pitch < args->width * DIV_ROUND_UP(args->bpp, 8))
> > > +		args->pitch = args->width * DIV_ROUND_UP(args->bpp, 8);
> > 
> > Shouldn't this be DIV_ROUND_UP(args->width * args->bpp, 8) ? Not all
> > formats might need to pad pixels to an integer number of bytes.
> 
> Are you thinking about YUV formats? I can't imagine RGB formats where
> this might be an issue.

Yes I was thinking mainly about YUV, although I'm not sure whether we already 
have formats where this could happen. It won't hurt to be prepared for the 
future though :-)

> Integrated the rest of you comments.

-- 
Regards,

Laurent Pinchart