dri-devel.lists.freedesktop.org archive mirror
 help / color / mirror / Atom feed
From: Brian Starkey <Brian.Starkey@arm.com>
To: John Stultz <john.stultz@linaro.org>
Cc: lkml <linux-kernel@vger.kernel.org>,
	"Andrew F. Davis" <afd@ti.com>, Laura Abbott <labbott@redhat.com>,
	Benjamin Gaignard <benjamin.gaignard@linaro.org>,
	Greg KH <gregkh@linuxfoundation.org>,
	Sumit Semwal <sumit.semwal@linaro.org>,
	Liam Mark <lmark@codeaurora.org>, Chenbo Feng <fengc@google.com>,
	Alistair Strachan <astrachan@google.com>,
	"dri-devel@lists.freedesktop.org"
	<dri-devel@lists.freedesktop.org>, nd <nd@arm.com>
Subject: Re: [RFC][PATCH 1/5 v2] dma-buf: Add dma-buf heaps framework
Date: Tue, 19 Mar 2019 12:08:26 +0000	[thread overview]
Message-ID: <20190319120825.3mvdxp5saluboy7o@DESKTOP-E1NTVVP.localdomain> (raw)
In-Reply-To: <1551819273-640-2-git-send-email-john.stultz@linaro.org>

Hi John,

On Tue, Mar 05, 2019 at 12:54:29PM -0800, John Stultz wrote:
> From: "Andrew F. Davis" <afd@ti.com>

[snip]

> +
> +#define NUM_HEAP_MINORS 128
> +static DEFINE_IDR(dma_heap_idr);
> +static DEFINE_MUTEX(minor_lock); /* Protect idr accesses */

I saw that Matthew Wilcox is trying to nuke idr:
https://patchwork.freedesktop.org/series/57073/

Perhaps a different data structure could be considered? (I don't have
an informed opinion on which).

> +
> +dev_t dma_heap_devt;
> +struct class *dma_heap_class;
> +struct list_head dma_heap_list;
> +struct dentry *dma_heap_debug_root;
> +
> +static int dma_heap_buffer_alloc(struct dma_heap *heap, size_t len,
> +				 unsigned int flags)
> +{
> +	len = PAGE_ALIGN(len);
> +	if (!len)
> +		return -EINVAL;

I think aligning len to pages only makes sense if heaps are going to
allocate aligned to pages too. Perhaps that's an implicit assumption?
If so, lets document it.

Why not let the heaps take care of aligning len however they want
though?

...

> +
> +int dma_heap_add(struct dma_heap *heap)
> +{
> +	struct device *dev_ret;
> +	int ret;
> +
> +	if (!heap->name || !strcmp(heap->name, "")) {
> +		pr_err("dma_heap: Cannot add heap without a name\n");
> +		return -EINVAL;
> +	}
> +
> +	if (!heap->ops || !heap->ops->allocate) {
> +		pr_err("dma_heap: Cannot add heap with invalid ops struct\n");
> +		return -EINVAL;
> +	}
> +
> +	/* Find unused minor number */
> +	mutex_lock(&minor_lock);
> +	ret = idr_alloc(&dma_heap_idr, heap, 0, NUM_HEAP_MINORS, GFP_KERNEL);
> +	mutex_unlock(&minor_lock);
> +	if (ret < 0) {
> +		pr_err("dma_heap: Unable to get minor number for heap\n");
> +		return ret;
> +	}
> +	heap->minor = ret;
> +
> +	/* Create device */
> +	heap->heap_devt = MKDEV(MAJOR(dma_heap_devt), heap->minor);
> +	dev_ret = device_create(dma_heap_class,
> +				NULL,
> +				heap->heap_devt,
> +				NULL,
> +				heap->name);
> +	if (IS_ERR(dev_ret)) {
> +		pr_err("dma_heap: Unable to create char device\n");
> +		return PTR_ERR(dev_ret);
> +	}
> +
> +	/* Add device */
> +	cdev_init(&heap->heap_cdev, &dma_heap_fops);
> +	ret = cdev_add(&heap->heap_cdev, dma_heap_devt, NUM_HEAP_MINORS);

Shouldn't this be s/dma_heap_devt/heap->heap_devt/ and a count of 1?

Also would it be better to have cdev_add/device_create the other way
around? First create the char device, then once it's all set up
register it with sysfs.

> +	if (ret < 0) {
> +		device_destroy(dma_heap_class, heap->heap_devt);
> +		pr_err("dma_heap: Unable to add char device\n");
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(dma_heap_add);

Until we've figured out how modules are going to work, I still think
it would be a good idea to not export this.

Cheers,
-Brian

  parent reply	other threads:[~2019-03-19 12:08 UTC|newest]

Thread overview: 69+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-03-05 20:54 [RFC][PATCH 0/5 v2] DMA-BUF Heaps (destaging ION) John Stultz
2019-03-05 20:54 ` [RFC][PATCH 1/5 v2] dma-buf: Add dma-buf heaps framework John Stultz
2019-03-06 16:12   ` Benjamin Gaignard
2019-03-06 16:57     ` John Stultz
2019-03-15  8:55     ` Christoph Hellwig
2019-03-06 16:27   ` Andrew F. Davis
2019-03-06 19:03     ` John Stultz
2019-03-06 21:45       ` Andrew F. Davis
2019-03-15  8:54   ` Christoph Hellwig
2019-03-15 20:24     ` Andrew F. Davis
2019-03-15 20:18   ` Laura Abbott
2019-03-15 20:49     ` Andrew F. Davis
2019-03-15 21:29     ` John Stultz
2019-03-15 22:44       ` Laura Abbott
2019-03-18  4:38         ` Sumit Semwal
2019-03-18  4:41         ` Sumit Semwal
2019-03-19 12:08   ` Brian Starkey [this message]
2019-03-19 15:24     ` Andrew F. Davis
2019-03-21 21:16     ` John Stultz
2019-03-27 14:53   ` Greg KH
2019-03-28  6:09     ` John Stultz
2019-03-05 20:54 ` [RFC][PATCH 2/5 v2] dma-buf: heaps: Add heap helpers John Stultz
2019-03-13 20:18   ` Liam Mark
2019-03-13 21:48     ` Andrew F. Davis
2019-03-13 22:57       ` Liam Mark
2019-03-13 23:42         ` Andrew F. Davis
2019-03-15  9:06   ` Christoph Hellwig
2019-03-19 15:03     ` Andrew F. Davis
2019-03-21 20:01     ` John Stultz
2019-03-19 14:26   ` Brian Starkey
2019-03-21 20:11     ` John Stultz
2019-03-21 20:35     ` Andrew F. Davis
2019-03-21 20:43   ` Andrew F. Davis
2019-03-05 20:54 ` [RFC][PATCH 3/5 v2] dma-buf: heaps: Add system heap to dmabuf heaps John Stultz
2019-03-06 16:01   ` Benjamin Gaignard
2019-03-11  5:48     ` John Stultz
2019-03-13 20:20   ` Liam Mark
2019-03-13 22:49     ` John Stultz
2019-03-15  9:06   ` Christoph Hellwig
2019-03-05 20:54 ` [RFC][PATCH 4/5 v2] dma-buf: heaps: Add CMA heap to dmabuf heapss John Stultz
2019-03-06 16:05   ` Benjamin Gaignard
2019-03-21 20:15     ` John Stultz
2019-03-15  9:06   ` Christoph Hellwig
2019-03-15 20:08     ` John Stultz
2019-03-19 14:53   ` Brian Starkey
2019-03-05 20:54 ` [RFC][PATCH 5/5 v2] kselftests: Add dma-heap test John Stultz
2019-03-06 16:14   ` Benjamin Gaignard
2019-03-06 16:35     ` Andrew F. Davis
2019-03-06 18:19       ` John Stultz
2019-03-06 18:32         ` Andrew F. Davis
2019-03-06 17:01     ` John Stultz
2019-03-15 20:07       ` Laura Abbott
2019-03-15 20:13         ` John Stultz
2019-03-15 20:49           ` Laura Abbott
2019-03-13 20:23   ` Liam Mark
2019-03-13 20:11 ` [RFC][PATCH 0/5 v2] DMA-BUF Heaps (destaging ION) Liam Mark
2019-03-13 22:30   ` John Stultz
2019-03-13 23:29     ` Liam Mark
2019-03-19 16:54     ` Benjamin Gaignard
2019-03-19 16:59       ` Andrew F. Davis
2019-03-19 21:58         ` Rob Clark
2019-03-19 22:36           ` John Stultz
2019-03-20  9:16             ` Benjamin Gaignard
2019-03-20 14:44               ` Andrew F. Davis
2019-03-20 15:59                 ` Benjamin Gaignard
2019-03-20 16:11               ` John Stultz
2019-03-15 20:34 ` Laura Abbott
2019-03-15 23:15 ` Jerome Glisse
2019-03-16  0:16   ` John Stultz

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190319120825.3mvdxp5saluboy7o@DESKTOP-E1NTVVP.localdomain \
    --to=brian.starkey@arm.com \
    --cc=afd@ti.com \
    --cc=astrachan@google.com \
    --cc=benjamin.gaignard@linaro.org \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=fengc@google.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=john.stultz@linaro.org \
    --cc=labbott@redhat.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=lmark@codeaurora.org \
    --cc=nd@arm.com \
    --cc=sumit.semwal@linaro.org \
    --subject='Re: [RFC][PATCH 1/5 v2] dma-buf: Add dma-buf heaps framework' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).