Re: [PATCH] omap2+: add drm device

[Tomi Valkeinen <tomi.valkeinen@ti.com>]

[-- Attachment #1: Type: text/plain, Size: 3362 bytes --]

On Tue, 2012-03-06 at 09:29 -0600, Rob Clark wrote:
> On Tue, Mar 6, 2012 at 8:35 AM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> > On Tue, 2012-03-06 at 08:01 -0600, Rob Clark wrote:
> >> On Tue, Mar 6, 2012 at 7:26 AM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> >
> >> > Can there be more than one omapdrm device? I guess not. If so, the id
> >> > should be -1.
> >>
> >> in the past, we have used multiple devices (using the platform-data to
> >> divide up the dss resources), although this was sort of a
> >> special-case.  But in theory you could have multiple.  (Think of a
> >> multi-seat scenario, where multiple compositors need to run and each
> >> need to be drm-master of their own device to do mode-setting on their
> >> corresponding display.)
> >>
> >> I think if we use .id = -1, then we'd end up with a funny looking
> >> bus-id for the device: "platform:omapdrm:-1"
> >
> > I don't know about that, but it's the way platform devices are meant to
> > be used if there can be only one instance. If the case where there are
> > multiple omapdrm devices is rather theoretical, or only used for certain
> > debugging scenarios or such, I think having the id as -1 in the mainline
> > is cleaner.
> 
> well, I don't care much one way or another, but need to check if there
> is a small patch needed in drm core code that generates the bus-id for
> .id = -1..

Hmm, why does the drm core care about it?

And generally, I think if the bus id is -1, there is no bus id in a
sense. For example, with bus id 0 you'll get a device "omapdrm.0". With
bus id -1 you'll get a device "omapdrm".

> >> >> +arch_initcall(omap_init_gpu);
> >> >> +
> >> >> +void omapdrm_reserve_vram(void)
> >> >> +{
> >> >> +#ifdef CONFIG_CMA
> >> >> +     /*
> >> >> +      * Create private 32MiB contiguous memory area for omapdrm.0 device
> >> >> +      * TODO revisit size.. if uc/wc buffers are allocated from CMA pages
> >> >> +      * then the amount of memory we need goes up..
> >> >> +      */
> >> >> +     dma_declare_contiguous(&omap_drm_device.dev, 32 * SZ_1M, 0, 0);
> >> >
> >> > What does this actually do? Does it reserve the memory, i.e. it's not
> >> > usable for others? If so, shouldn't there be a way for the user to
> >> > configure it?
> >>
> >> It can be re-used by others.. see http://lwn.net/Articles/479297/
> >
> > The link didn't tell much. I looked at the patches, and
> > dma_declare_contiguous allocates the memory with memblock_alloc. So is
> > that allocated memory available for any users? If so, why does the
> > function want a device pointer as an argument...
> >
> > Is the memory available for normal kmalloc, or only available via the
> > CMA functions? Surely there must be some downside to the above? =) And
> > if so, it should be configurable. You could have a board with no display
> > at all, and you probably don't want to have 32MB allocated for DRM in
> > that case.
> 
> Basically the short version is memory from a CMA carveout can be
> re-used for unpinnable memory.  Not kmalloc, but it can be used for
> anon userspace pages, for example.  Userspace needs memory too.

Okay, let me ask the other way. Is 32MB enough for everyone? Hardcoding
a value like that without any possibility to adjust it just sounds like
a rather bad thing.

 Tomi


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]