* Re: [RFC 0/1] drm/pl111: Initial drm/kms driver for pl111
From: Rob Clark @ 2013-08-06 19:06 UTC (permalink / raw)
To: Tom Cooksey
Cc: dri-devel, linux-fbdev, Pawel Moll, linux-arm-kernel, linux-media,
linaro-mm-sig, linux-kernel
In-Reply-To: <52013482.e107c20a.27f9.ffffa718SMTPIN_ADDED_BROKEN@mx.google.com>
On Tue, Aug 6, 2013 at 1:38 PM, Tom Cooksey <tom.cooksey@arm.com> wrote:
>
>> >> ... This is the purpose of the attach step,
>> >> so you know all the devices involved in sharing up front before
>> >> allocating the backing pages. (Or in the worst case, if you have a
>> >> "late attacher" you at least know when no device is doing dma access
>> >> to a buffer and can reallocate and move the buffer.) A long time
>> >> back, I had a patch that added a field or two to 'struct
>> >> device_dma_parameters' so that it could be known if a device
>> >> required contiguous buffers.. looks like that never got merged, so
>> >> I'd need to dig that back up and resend it. But the idea was to
>> >> have the 'struct device' encapsulate all the information that would
>> >> be needed to do-the-right-thing when it comes to placement.
>> >
>> > As I understand it, it's up to the exporting device to allocate the
>> > memory backing the dma_buf buffer. I guess the latest possible point
>> > you can allocate the backing pages is when map_dma_buf is first
>> > called? At that point the exporter can iterate over the current set
>> > of attachments, programmatically determine the all the constraints of
>> > all the attached drivers and attempt to allocate the backing pages
>> > in such a way as to satisfy all those constraints?
>>
>> yes, this is the idea.. possibly some room for some helpers to help
>> out with this, but that is all under the hood from userspace
>> perspective
>>
>> > Didn't you say that programmatically describing device placement
>> > constraints was an unbounded problem? I guess we would have to
>> > accept that it's not possible to describe all possible constraints
>> > and instead find a way to describe the common ones?
>>
>> well, the point I'm trying to make, is by dividing your constraints
>> into two groups, one that impacts and is handled by userspace, and one
>> that is in the kernel (ie. where the pages go), you cut down the
>> number of permutations that the kernel has to care about considerably.
>> And kernel already cares about, for example, what range of addresses
>> that a device can dma to/from. I think really the only thing missing
>> is the max # of sglist entries (contiguous or not)
>
> I think it's more than physically contiguous or not.
>
> For example, it can be more efficient to use large page sizes on
> devices with IOMMUs to reduce TLB traffic. I think the size and even
> the availability of large pages varies between different IOMMUs.
sure.. but I suppose if we can spiff out dma_params to express "I need
contiguous", perhaps we can add some way to express "I prefer
as-contiguous-as-possible".. either way, this is about where the pages
are placed, and not about the layout of pixels within the page, so
should be in kernel. It's something that is missing, but I believe
that it belongs in dma_params and hidden behind dma_alloc_*() for
simple drivers.
> There's also the issue of buffer stride alignment. As I say, if the
> buffer is to be written by a tile-based GPU like Mali, it's more
> efficient if the buffer's stride is aligned to the max AXI bus burst
> length. Though I guess a buffer stride only makes sense as a concept
> when interpreting the data as a linear-layout 2D image, so perhaps
> belongs in user-space along with format negotiation?
>
Yeah.. this isn't about where the pages go, but about the arrangement
within a page.
And, well, except for hw that supports the same tiling (or
compressed-fb) in display+gpu, you probably aren't sharing tiled
buffers.
>
>> > One problem with this is it duplicates a lot of logic in each
>> > driver which can export a dma_buf buffer. Each exporter will need to
>> > do pretty much the same thing: iterate over all the attachments,
>> > determine of all the constraints (assuming that can be done) and
>> > allocate pages such that the lowest-common-denominator is satisfied.
>> >
>> > Perhaps rather than duplicating that logic in every driver, we could
>> > Instead move allocation of the backing pages into dma_buf itself?
>> >
>>
>> I tend to think it is better to add helpers as we see common patterns
>> emerge, which drivers can opt-in to using. I don't think that we
>> should move allocation into dma_buf itself, but it would perhaps be
>> useful to have dma_alloc_*() variants that could allocate for multiple
>> devices.
>
> A helper could work I guess, though I quite like the idea of having
> dma_alloc_*() variants which take a list of devices to allocate memory
> for.
>
>
>> That would help for simple stuff, although I'd suspect
>> eventually a GPU driver will move away from that. (Since you probably
>> want to play tricks w/ pools of pages that are pre-zero'd and in the
>> correct cache state, use spare cycles on the gpu or dma engine to
>> pre-zero uncached pages, and games like that.)
>
> So presumably you're talking about a GPU driver being the exporter
> here? If so, how could the GPU driver do these kind of tricks on
> memory shared with another device?
>
Yes, that is gpu-as-exporter. If someone else is allocating buffers,
it is up to them to do these tricks or not. Probably there is a
pretty good chance that if you aren't a GPU you don't need those sort
of tricks for fast allocation of transient upload buffers, staging
textures, temporary pixmaps, etc. Ie. I don't really think a v4l
camera or video decoder would benefit from that sort of optimization.
>
>> >> > Anyway, assuming user-space can figure out how a buffer should be
>> >> > stored in memory, how does it indicate this to a kernel driver and
>> >> > actually allocate it? Which ioctl on which device does user-space
>> >> > call, with what parameters? Are you suggesting using something
>> >> > like ION which exposes the low-level details of how buffers are
>> > >> laid out in physical memory to userspace? If not, what?
>> >> >
>> >>
>> >> no, userspace should not need to know this. And having a central
>> >> driver that knows this for all the other drivers in the system
>> >> doesn't really solve anything and isn't really scalable. At best
>> >> you might want, in some cases, a flag you can pass when allocating.
>> >> For example, some of the drivers have a 'SCANOUT' flag that can be
>> >> passed when allocating a GEM buffer, as a hint to the kernel that
>> >> 'if this hw requires contig memory for scanout, allocate this
>> >> buffer contig'. But really, when it comes to sharing buffers
>> >> between devices, we want this sort of information in dev->dma_params
>> >> of the importing device(s).
>> >
>> > If you had a single driver which knew the constraints of all devices
>> > on that particular SoC and the interface allowed user-space to
>> > specify which devices a buffer is intended to be used with, I guess
>> > it could pretty trivially allocate pages which satisfy those
> constraints?
>>
>> keep in mind, even a number of SoC's come with pcie these days. You
>> already have things like
>>
>> https://developer.nvidia.com/content/kayla-platform
>>
>> You probably want to get out of the SoC mindset, otherwise you are
>> going to make bad assumptions that come back to bite you later on.
>
> Sure - there are always going to be PC-like devices where the
> hardware configuration isn't fixed like it is on a traditional SoC.
> But I'd rather have a simple solution which works on traditional SoCs
> than no solution at all. Today our solution is to over-load the dumb
> buffer alloc functions of the display's DRM driver - For now I'm just
> looking for the next step up from that! ;-)
>
True.. the original intention, which is perhaps a bit desktop-centric,
really was for there to be a userspace component talking to the drm
driver for allocation, ie. xf86-video-foo and/or
src/gallium/drivers/foo (for example) ;-)
Which means for x11 having a SoC vendor specific xf86-video-foo for
x11.. or vendor specific gbm implementation for wayland. (Although
at least in the latter case it is a pretty small piece of code.) But
that is probably what you are trying to avoid.
At any rate, for both xorg and wayland/gbm, you know when a buffer is
going to be a scanout buffer. What I'd recommend is define a small
userspace API that your customers (the SoC vendors) implement to
allocate a scanout buffer and hand you back a dmabuf fd. That could
be used both for x11 and for gbm. Inputs should be requested
width/height and format. And outputs pitch plus dmabuf fd.
(Actually you might even just want to use gbm as your starting point.
You could probably just use gbm from xf86-video-armsoc for allocation,
to have one thing that works for both wayland and x11. Scanout and
cursor buffers should go to vendor/SoC specific fxn, rest can be
allocated from mali kernel driver.)
>
>> > wouldn't need a way to programmatically describe the constraints
>> > either: As you say, if userspace sets the "SCANOUT" flag, it would
>> > just "know" that on this SoC, that buffer needs to be physically
>> > contiguous for example.
>>
>> not really.. it just knows it wants to scanout the buffer, and tells
>> this as a hint to the kernel.
>>
>> For example, on omapdrm, the SCANOUT flag does nothing on omap4+
>> (where phys contig is not required for scanout), but causes CMA
>> (dma_alloc_*()) to be used on omap3. Userspace doesn't care. It just
>> knows that it wants to be able to scanout that particular buffer.
>
> I think that's the idea? The omap3's allocator driver would use
> contiguous memory when it detects the SCANOUT flag whereas the omap4
> allocator driver wouldn't have to. No complex negotiation of
> constraints - it just "knows".
>
well, it is same allocating driver in both cases (although maybe that
is unimportant). The "it" that just knows it wants to scanout is
userspace. The "it" that just knows that scanout translates to
contiguous (or not) is the kernel. Perhaps we are saying the same
thing ;-)
BR,
-R
>
> Cheers,
>
> Tom
>
>
>
>
^ permalink raw reply
* [PATCH] efifb: prevent null dereferences by removing unused array indices from dmi_list
From: James Bates @ 2013-08-06 23:15 UTC (permalink / raw)
To: Peter Jones, Jean-Christophe Plagniol-Villard, Tomi Valkeinen,
linux-fbdev, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 2368 bytes --]
Hi all,
The dmi_list array is initialized using gnu designated initializers, and therefore
contains fewer explicitly defined entries as there are elements in it. This
is because the enum above with M_blabla constants contains more items than the designated
initializer. Those elements not explicitly initialized are implicitly set to 0.
Now efifb_setup(), L.322 & L.323, loops through all these array elements, and performs
a strcmp o a field (optname) in each item. For non explicitly initialized elements this
will be a null pointer:
for (i = 0; i < M_UNKNOWN; i++) {
if (!strcmp(this_opt, dmi_list[i].optname) &&
On my macbook6,1 the predefined values are for some reason incorrect, and most parameters
are preset correctly by my efi bootloader (elilo). but stride/line_length is not detected
correctly and so I wish to set it explicitly using a "video=efifb:stride:2048" command-line
argument. Because of the above null dereference, an exception (presumably) occurs before
the parsing code (L.333) is ever reached. I say presumably since the mac hangs on boot
without a console, and I can therefore not see any output.
By removing the unused values from the enum, and thus preventing implicitly initialized items
in the dmi_list array, the null dereference does not occur, my customer command-line arg is
parsed correctly, and my console displays correctly.
Signed-off-by: James Bates <james.h.bates@gmail.com>
---
drivers/video/efifb.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/drivers/video/efifb.c b/drivers/video/efifb.c
index 50fe668..52d1d88 100644
--- a/drivers/video/efifb.c
+++ b/drivers/video/efifb.c
@@ -50,12 +50,9 @@ enum {
M_MINI_3_1, /* Mac Mini, 3,1th gen */
M_MINI_4_1, /* Mac Mini, 4,1th gen */
M_MB, /* MacBook */
- M_MB_2, /* MacBook, 2nd rev. */
- M_MB_3, /* MacBook, 3rd rev. */
M_MB_5_1, /* MacBook, 5th rev. */
M_MB_6_1, /* MacBook, 6th rev. */
M_MB_7_1, /* MacBook, 7th rev. */
- M_MB_SR, /* MacBook, 2nd gen, (Santa Rosa) */
M_MBA, /* MacBook Air */
M_MBA_3, /* Macbook Air, 3rd rev */
M_MBP, /* MacBook Pro */
--
1.7.12.4 (Apple Git-37)
[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 4151 bytes --]
^ permalink raw reply related
* Re: [RFC 0/1] drm/pl111: Initial drm/kms driver for pl111
From: John Stultz @ 2013-08-07 3:57 UTC (permalink / raw)
To: Rob Clark
Cc: linux-fbdev, Erik Gilling, Pawel Moll, dri-devel, Ross Oldfield,
Tom Cooksey, Rebecca Schultz Zavin, linux-arm-kernel
In-Reply-To: <CAF6AEGvFPGueM_LHVij9KFzM6NJySHCzmaLstuzZkK5GwP+6gQ@mail.gmail.com>
On Mon, Jul 29, 2013 at 7:58 AM, Rob Clark <robdclark@gmail.com> wrote:
> On Fri, Jul 26, 2013 at 11:58 AM, Tom Cooksey <tom.cooksey@arm.com> wrote:
>> Hi Rob,
>>
>>> > * It abuses flags parameter of DRM_IOCTL_MODE_CREATE_DUMB to also
>>> > allocate buffers for the GPU. Still not sure how to resolve this
>>> > as we don't use DRM for our GPU driver.
>>>
>>> any thoughts/plans about a DRM GPU driver? Ideally long term (esp.
>>> once the dma-fence stuff is in place), we'd have gpu-specific drm
>>> (gpu-only, no kms) driver, and SoC/display specific drm/kms driver,
>>> using prime/dmabuf to share between the two.
>>
>> The "extra" buffers we were allocating from armsoc DDX were really
>> being allocated through DRM/GEM so we could get an flink name
>> for them and pass a reference to them back to our GPU driver on
>> the client side. If it weren't for our need to access those
>> extra off-screen buffers with the GPU we wouldn't need to
>> allocate them with DRM at all. So, given they are really "GPU"
>> buffers, it does absolutely make sense to allocate them in a
>> different driver to the display driver.
>>
>> However, to avoid unnecessary memcpys & related cache
>> maintenance ops, we'd also like the GPU to render into buffers
>> which are scanned out by the display controller. So let's say
>> we continue using DRM_IOCTL_MODE_CREATE_DUMB to allocate scan
>> out buffers with the display's DRM driver but a custom ioctl
>> on the GPU's DRM driver to allocate non scanout, off-screen
>> buffers. Sounds great, but I don't think that really works
>> with DRI2. If we used two drivers to allocate buffers, which
>> of those drivers do we return in DRI2ConnectReply? Even if we
>> solve that somehow, GEM flink names are name-spaced to a
>> single device node (AFAIK). So when we do a DRI2GetBuffers,
>> how does the EGL in the client know which DRM device owns GEM
>> flink name "1234"? We'd need some pretty dirty hacks.
>
> You would return the name of the display driver allocating the
> buffers. On the client side you can use generic ioctls to go from
> flink -> handle -> dmabuf. So the client side would end up opening
> both the display drm device and the gpu, but without needing to know
> too much about the display.
>
> (Probably in (for example) mesa there needs to be a bit of work to
> handle this better, but I think that would be needed as well for
> sharing between, say, nouveau and udl displaylink driver.. which is
> really the same scenario.)
>
>> So then we looked at allocating _all_ buffers with the GPU's
>> DRM driver. That solves the DRI2 single-device-name and single
>> name-space issue. It also means the GPU would _never_ render
>> into buffers allocated through DRM_IOCTL_MODE_CREATE_DUMB.
>
> Well, I think we can differentiate between shared buffers and internal
> buffers (textures, vertex upload, etc, etc).
>
> For example, in mesa/gallium driver there are two paths to getting a
> buffer... ->resource_create() and ->resource_from_handle(), the
> latter is the path you go for dri2 buffers shared w/ x11. The former
> is buffers that are just internal to gpu (if !(bind &
> PIPE_BIND_SHARED)). I guess you must have something similar in mali
> driver.
>
>> One thing I wasn't sure about is if there was an objection
>> to using PRIME to export scanout buffers allocated with
>> DRM_IOCTL_MODE_CREATE_DUMB and then importing them into a GPU
>> driver to be rendered into? Is that a concern?
>
> well.. it wasn't quite the original intention of the 'dumb' ioctls.
> But I guess in the end if you hand the gpu a buffer with a
> layout/format that it can't grok, then gpu does a staging texture plus
> copy. If you had, for example, some setup w/ gpu that could only
> render to tiled format, plus a display that could scanout that format,
> then your DDX driver would need to allocate the dri2 buffers with
> something other than dumb ioctl. (But at that point, you probably
> need to implement your own EXA so you could hook in your own custom
> upload-to/download-from screen fxns for sw fallbacks, so you're not
> talking about using generic DDX anyways.)
>
>> Anyway, that latter case also gets quite difficult. The "GPU"
>> DRM driver would need to know the constraints of the display
>> controller when allocating buffers intended to be scanned out.
>> For example, pl111 typically isn't behind an IOMMU and so
>> requires physically contiguous memory. We'd have to teach the
>> GPU's DRM driver about the constraints of the display HW. Not
>> exactly a clean driver model. :-(
>>
>> I'm still a little stuck on how to proceed, so any ideas
>> would greatly appreciated! My current train of thought is
>> having a kind of SoC-specific DRM driver which allocates
>> buffers for both display and GPU within a single GEM
>> namespace. That SoC-specific DRM driver could then know the
>> constraints of both the GPU and the display HW. We could then
>> use PRIME to export buffers allocated with the SoC DRM driver
>> and import them into the GPU and/or display DRM driver.
>
> Usually if the display drm driver is allocating the buffers that might
> be scanned out, it just needs to have minimal knowledge of the GPU
> (pitch alignment constraints). I don't think we need a 3rd device
> just to allocate buffers.
>
> Really I don't think the separate display drm and gpu drm device is
> much different from desktop udl/displaylink case. Or desktop
> integrated-gpu + external gpu.
Although on many SoCs, aren't the pipelines a bit more complicated?
(ie: camera data -> doing face detection, -> applying filters w/ the
gpu -> and displaying it).
While ION does have interface issues (in that it requires userland to
hard-code particular constraints for that specific SoC) just the idea
of having the centralized allocator that manages the constraints does
seem to make sense for these larger pipelines (and also the fact that
there's different allocation APIs for various devices).
One idea that was discussed in some of the hallway discussions at
connect was if there was some way for the hardware to expose a
"constraint cookie" via sysfs (basically a bit field where the meaning
of each bit is opaque to userland - so each bit can mean a different
constraint on different devices), userland could determine which
combination of devices it wanted to pipe a buffer through, then AND
the device constraint cookies together, and pass that to a
centralized allocator. This allows the constraint solving to be pushed
out to userland, but also avoids what I see as the biggest negatives
in ION (ie: the heap mask is defined to userland, making it
inflexible, as well as the lack of a method for userland to discover
device constraints, so they're instead hard-coded in userland for each
device).
>> Note: While it doesn't use the DRM framework, the Mali T6xx
>> kernel driver has supported importing buffers through dma_buf
>> for some time. I've even written an EGL extension :-):
>>
>> <http://www.khronos.org/registry/egl/extensions/EXT/EGL_EXT_image_dma_buf_im
>> port.txt>
>>
>>
>>> I'm not entirely sure that the directions that the current CDF
>>> proposals are headed is necessarily the right way forward. I'd prefer
>>> to see small/incremental evolution of KMS (ie. add drm_bridge and
>>> drm_panel, and refactor the existing encoder-slave). Keeping it
>>> inside drm means that we can evolve it more easily, and avoid layers
>>> of glue code for no good reason.
>>
>> I think CDF could allow vendors to re-use code they've written
>> for their Android driver stack in DRM drivers more easily. Though
>> I guess ideally KMS would evolve to a point where it could be used
>> by an Android driver stack. I.e. Support explicit fences.
>>
>
> yeah, the best would be evolving DRM/KMS to the point that it meets
> android requirements. Because I really don't think android has any
> special requirements compared to, say, a wayland compositor. (Other
> than the way they do synchronization. But that doesn't really *need*
> to be different, as far as I can tell.) It would certainly be easier
> if android dev's actually participated in upstream linux graphics, and
> talked about what they needed, and maybe even contributed a patch or
> two. But that is a whole different topic.
It also couldn't hurt to cc them on these discussions (Rebecca and Erik cc'ed :)
I think Ross covered much of the KMS issues at Linaro Connect (Video
here if you want to spend the time: http://youtu.be/AuoFABKS_Dk), and
indeed, the explicit syncing requirement is probably the biggest
barrier. My understanding is the additional parallelism/performance
that the explicit method provides is the key requirement (though maybe
Erik or Ross can chime in with further details? - I've been told Erik
has a long list as to why explicit is preferred over implicit and it
would be good to get that documented somewhere :)
I understand the implicit model is easier to program for, and Daniel
mentioned dynamic buffer eviction as having a requirement on the
implicit model, so I'm currently in the camp of assuming the
synchronization needs are different enough that one method alone (of
the two currently presented) likely won't work. My current hope is
there's some hybrid approach that can be used (implicitly maybe for
the majority of cases, but with the option of doing explicit syncing
when necessary).
thanks
-john (who's unfortunately heading off on vacation soon, so might not
be fast to reply)
^ permalink raw reply
* Re: [RFC 0/1] drm/pl111: Initial drm/kms driver for pl111
From: John Stultz @ 2013-08-07 4:23 UTC (permalink / raw)
To: Rob Clark
Cc: linux-fbdev, Erik Gilling, Pawel Moll, lkml, dri-devel,
linaro-mm-sig, Ross Oldfield, Tom Cooksey, Rebecca Schultz Zavin,
linux-arm-kernel, linux-media
In-Reply-To: <CAF6AEGvXcpTKrTjhvrycLqab6F9QP5fAk0ZEWxJ-WvE==PiPsA@mail.gmail.com>
On Tue, Aug 6, 2013 at 5:15 AM, Rob Clark <robdclark@gmail.com> wrote:
> On Tue, Aug 6, 2013 at 7:31 AM, Tom Cooksey <tom.cooksey@arm.com> wrote:
>>
>>> > So in some respects, there is a constraint on how buffers which will
>>> > be drawn to using the GPU are allocated. I don't really like the idea
>>> > of teaching the display controller DRM driver about the GPU buffer
>>> > constraints, even if they are fairly trivial like this. If the same
>>> > display HW IP is being used on several SoCs, it seems wrong somehow
>>> > to enforce those GPU constraints if some of those SoCs don't have a
>>> > GPU.
>>>
>>> Well, I suppose you could get min_pitch_alignment from devicetree, or
>>> something like this..
>>>
>>> In the end, the easy solution is just to make the display allocate to
>>> the worst-case pitch alignment. In the early days of dma-buf
>>> discussions, we kicked around the idea of negotiating or
>>> programatically describing the constraints, but that didn't really
>>> seem like a bounded problem.
>>
>> Yeah - I was around for some of those discussions and agree it's not
>> really an easy problem to solve.
>>
>>
>>
>>> > We may also then have additional constraints when sharing buffers
>>> > between the display HW and video decode or even camera ISP HW.
>>> > Programmatically describing buffer allocation constraints is very
>>> > difficult and I'm not sure you can actually do it - there's some
>>> > pretty complex constraints out there! E.g. I believe there's a
>>> > platform where Y and UV planes of the reference frame need to be in
>>> > separate DRAM banks for real-time 1080p decode, or something like
>>> > that?
>>>
>>> yes, this was discussed. This is different from pitch/format/size
>>> constraints.. it is really just a placement constraint (ie. where do
>>> the physical pages go). IIRC the conclusion was to use a dummy
>>> devices with it's own CMA pool for attaching the Y vs UV buffers.
>>>
>>> > Anyway, I guess my point is that even if we solve how to allocate
>>> > buffers which will be shared between the GPU and display HW such that
>>> > both sets of constraints are satisfied, that may not be the end of
>>> > the story.
>>> >
>>>
>>> that was part of the reason to punt this problem to userspace ;-)
>>>
>>> In practice, the kernel drivers doesn't usually know too much about
>>> the dimensions/format/etc.. that is really userspace level knowledge.
>>> There are a few exceptions when the kernel needs to know how to setup
>>> GTT/etc for tiled buffers, but normally this sort of information is up
>>> at the next level up (userspace, and drm_framebuffer in case of
>>> scanout). Userspace media frameworks like GStreamer already have a
>>> concept of format/caps negotiation. For non-display<->gpu sharing, I
>>> think this is probably where this sort of constraint negotiation
>>> should be handled.
>>
>> I agree that user-space will know which devices will access the buffer
>> and thus can figure out at least a common pixel format. Though I'm not
>> so sure userspace can figure out more low-level details like alignment
>> and placement in physical memory, etc.
>
> well, let's divide things up into two categories:
>
> 1) the arrangement and format of pixels.. ie. what userspace would
> need to know if it mmap's a buffer. This includes pixel format,
> stride, etc. This should be negotiated in userspace, it would be
> crazy to try to do this in the kernel.
>
> 2) the physical placement of the pages. Ie. whether it is contiguous
> or not. Which bank the pages in the buffer are placed in, etc. This
> is not visible to userspace. This is the purpose of the attach step,
> so you know all the devices involved in sharing up front before
> allocating the backing pages. (Or in the worst case, if you have a
> "late attacher" you at least know when no device is doing dma access
> to a buffer and can reallocate and move the buffer.) A long time
One concern I know the Android folks have expressed previously (and
correct me if its no longer an objection), is that this attach time
in-kernel constraint solving / moving or reallocating buffers is
likely to hurt determinism. If I understood, their perspective was
that userland knows the device path the buffers will travel through,
so why not leverage that knowledge, rather then having the kernel have
to sort it out for itself after the fact.
The concern about determinism even makes them hesitant about CMA, over
things like carveout, as they don't want to be moving pages around at
allocation time (which could hurt reasonable use cases like the time
it takes to launch a camera app - which is quite important). Though
maybe this concern will lessen as more CMA solutions ship.
I worry some of this split between fully general solutions vs
hard-coded known constraints is somewhat intractable. But what might
make it easier to get android folks interested in approaches like the
attach-time allocation / relocating on late-attach you're proposing is
if there is maybe some way, as you suggested, to hint the allocation
when they do know the device paths, so they can provide that insight
and can avoid *all* reallocations.
Then of course, there's the question of how to consistently hint
things for all the different driver allocation interfaces (and that,
maybe naively, leads to the central allocator approach).
thanks
-john
^ permalink raw reply
* RE: [PATCH 4/5] fb: Add DCU framebuffer driver for Vybrid VF610 platform
From: Wang Huan-B18965 @ 2013-08-07 8:07 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1375778859.4276.25.camel@weser.hi.pengutronix.de>
DQo+IEFtIEZyZWl0YWcsIGRlbiAxMi4wNy4yMDEzLCAxNDowNyArMDgwMCBzY2hyaWViIEFsaXNv
biBXYW5nOg0KPiA+IFRoZSBEaXNwbGF5IENvbnRyb2xsZXIgVW5pdCAoRENVKSBtb2R1bGUgaXMg
YSBzeXN0ZW0gbWFzdGVyIHRoYXQNCj4gPiBmZXRjaGVzIGdyYXBoaWNzIHN0b3JlZCBpbiBpbnRl
cm5hbCBvciBleHRlcm5hbCBtZW1vcnkgYW5kIGRpc3BsYXlzDQo+ID4gdGhlbSBvbiBhIFRGVCBM
Q0QgcGFuZWwuIEEgd2lkZSByYW5nZSBvZiBwYW5lbCBzaXplcyBpcyBzdXBwb3J0ZWQgYW5kDQo+
ID4gdGhlIHRpbWluZyBvZiB0aGUgaW50ZXJmYWNlIHNpZ25hbHMgaXMgaGlnaGx5IGNvbmZpZ3Vy
YWJsZS4NCj4gPiBHcmFwaGljcyBhcmUgcmVhZCBkaXJlY3RseSBmcm9tIG1lbW9yeSBhbmQgdGhl
biBibGVuZGVkIGluIHJlYWwtdGltZSwNCj4gPiB3aGljaCBhbGxvd3MgZm9yIGR5bmFtaWMgY29u
dGVudCBjcmVhdGlvbiB3aXRoIG1pbmltYWwgQ1BVDQo+IGludGVydmVudGlvbi4NCj4gPg0KPiA+
IFRoZSBmZWF0dXJlczoNCj4gPg0KPiA+ICgxKSBGdWxsIFJHQjg4OCBvdXRwdXQgdG8gVEZUIExD
RCBwYW5lbC4NCj4gPiAoMikgRm9yIHRoZSBjdXJyZW50IExDRCBwYW5lbCwgV1FWR0EgIjQ4MHgy
NzIiIGlzIHRlc3RlZC4NCj4gPiAoMykgQmxlbmRpbmcgb2YgZWFjaCBwaXhlbCB1c2luZyB1cCB0
byA0IHNvdXJjZSBsYXllcnMgZGVwZW5kZW50IG9uDQo+IHNpemUgb2YgcGFuZWwuDQo+ID4gKDQp
IEVhY2ggZ3JhcGhpYyBsYXllciBjYW4gYmUgcGxhY2VkIHdpdGggb25lIHBpeGVsIHJlc29sdXRp
b24gaW4NCj4gZWl0aGVyIGF4aXMuDQo+ID4gKDUpIEVhY2ggZ3JhcGhpYyBsYXllciBzdXBwb3J0
IFJHQjU2NSBhbmQgUkdCODg4IGRpcmVjdCBjb2xvcnMNCj4gd2l0aG91dA0KPiA+IGFscGhhIGNo
YW5uZWwgYW5kIEJHUkE4ODg4IGRpcmVjdCBjb2xvcnMgd2l0aCBhbiBhbHBoYSBjaGFubmVsLg0K
PiA+ICg2KSBFYWNoIGdyYXBoaWMgbGF5ZXIgc3VwcG9ydCBhbHBoYSBibGVuZGluZyB3aXRoIDgt
Yml0IHJlc29sdXRpb24uDQo+ID4NCj4gPiBUaGlzIGRyaXZlciBoYXMgYmVlbiB0ZXN0ZWQgb24g
VnlicmlkIFZGNjEwIFRPV0VSIGJvYXJkLg0KPiA+DQo+ID4gU2lnbmVkLW9mZi1ieTogQWxpc29u
IFdhbmcgPGIxODk2NUBmcmVlc2NhbGUuY29tPg0KPiA+IC0tLQ0KPiA+IENoYW5nZXMgaW4gdjI6
IE5vbmUNCj4gPg0KPiA+ICBkcml2ZXJzL3ZpZGVvL0tjb25maWcgICAgICB8ICAgIDkgKw0KPiA+
ICBkcml2ZXJzL3ZpZGVvL01ha2VmaWxlICAgICB8ICAgIDEgKw0KPiA+ICBkcml2ZXJzL3ZpZGVv
L2ZzbC1kY3UtZmIuYyB8IDEwOTENCj4gPiArKysrKysrKysrKysrKysrKysrKysrKysrKysrKysr
KysrKysrKysrKysrKw0KPiA+ICAzIGZpbGVzIGNoYW5nZWQsIDExMDEgaW5zZXJ0aW9ucygrKQ0K
PiA+ICBjcmVhdGUgbW9kZSAxMDA2NDQgZHJpdmVycy92aWRlby9mc2wtZGN1LWZiLmMNCj4gPg0K
PiBbLi4uXQ0KPiA+ICtlbnVtIG1mYl9pbmRleCB7DQo+ID4gKwlMQVlFUjAgPSAwLA0KPiA+ICsJ
TEFZRVIxLA0KPiA+ICsJTEFZRVIyLA0KPiA+ICsJTEFZRVIzLA0KPiA+ICt9Ow0KPiBXaHkgYXJl
IHRoZXJlIG9ubHkgNCBsYXllcnMgaGVyZT8gSSB0aG91Z2h0IHRoZSBjb250cm9sbGVyIHN1cHBv
cnRzIGF0DQo+IGxlYXN0IDYgc2ltdWx0YW5lb3VzIGxheWVycz8NCltBbGlzb24gV2FuZ10gSSB1
c2VkIDQgbGF5ZXJzIGZvciB0aGUgY3VzdG9tZXIncyByZXF1aXJlbWVudCBiZWZvcmUuIEFueXdh
eSwgSSB3aWxsIGNoYW5nZSBpdCB0byA2IGluIG5leHQgdmVyc2lvbi4NCkJUVywgYWNjb3JkaW5n
IHRvIHRoZSBSTSwgaXQgc2FpZCAiQmxlbmRpbmcgb2YgZWFjaCBwaXhlbCB1c2luZyB1cCB0byA2
IHNvdXJjZSBsYXllcnMgZGVwZW5kZW50IG9uIHNpemUgb2YgcGFuZWwiLiBTbyBJIHRoaW5rIHRo
ZSBtb3N0IHNpbXVsdGFuZW91cyBsYXllcnMgaXMgNi4NCj4gDQo+ID4gKw0KPiA+ICtzdGF0aWMg
c3RydWN0IG1mYl9pbmZvIG1mYl90ZW1wbGF0ZVtdID0gew0KPiA+ICsJew0KPiA+ICsJLmluZGV4
ID0gTEFZRVIwLA0KPiA+ICsJLmlkID0gIkxheWVyMCIsDQo+ID4gKwkuYWxwaGEgPSAweGZmLA0K
PiA+ICsJLmJsZW5kID0gMCwNCj4gPiArCS5jb3VudCA9IDAsDQo+ID4gKwkueF9sYXllcl9kID0g
MCwNCj4gPiArCS55X2xheWVyX2QgPSAwLA0KPiA+ICsJfSwNCj4gPiArCXsNCj4gPiArCS5pbmRl
eCA9IExBWUVSMSwNCj4gPiArCS5pZCA9ICJMYXllcjEiLA0KPiA+ICsJLmFscGhhID0gMHhmZiwN
Cj4gPiArCS5ibGVuZCA9IDAsDQo+ID4gKwkuY291bnQgPSAwLA0KPiA+ICsJLnhfbGF5ZXJfZCA9
IDUwLA0KPiA+ICsJLnlfbGF5ZXJfZCA9IDUwLA0KPiA+ICsJfSwNCj4gPiArCXsNCj4gPiArCS5p
bmRleCA9IExBWUVSMiwNCj4gPiArCS5pZCA9ICJMYXllcjIiLA0KPiA+ICsJLmFscGhhID0gMHhm
ZiwNCj4gPiArCS5ibGVuZCA9IDAsDQo+ID4gKwkuY291bnQgPSAwLA0KPiA+ICsJLnhfbGF5ZXJf
ZCA9IDEwMCwNCj4gPiArCS55X2xheWVyX2QgPSAxMDAsDQo+ID4gKwl9LA0KPiA+ICsJew0KPiA+
ICsJLmluZGV4ID0gTEFZRVIzLA0KPiA+ICsJLmlkID0gIkxheWVyMyIsDQo+ID4gKwkuYWxwaGEg
PSAweGZmLA0KPiA+ICsJLmJsZW5kID0gMCwNCj4gPiArCS5jb3VudCA9IDAsDQo+ID4gKwkueF9s
YXllcl9kID0gMTUwLA0KPiA+ICsJLnlfbGF5ZXJfZCA9IDE1MCwNCj4gPiArCX0sDQo+ID4gK307
DQo+IFsuLi5dDQo+ID4gK3N0YXRpYyBpbnQgZnNsX2RjdV9yZWxlYXNlKHN0cnVjdCBmYl9pbmZv
ICppbmZvLCBpbnQgdXNlcikgew0KPiA+ICsJc3RydWN0IG1mYl9pbmZvICptZmJpID0gaW5mby0+
cGFyOw0KPiA+ICsJaW50IHJldCA9IDA7DQo+ID4gKw0KPiA+ICsJbWZiaS0+Y291bnQtLTsNCj4g
PiArCWlmIChtZmJpLT5jb3VudCA9PSAwKSB7DQo+ID4gKwkJcmV0ID0gZGlzYWJsZV9wYW5lbChp
bmZvKTsNCj4gPiArCQlpZiAocmV0IDwgMCkNCj4gPiArCQkJbWZiaS0+Y291bnQrKzsNCj4gPiAr
CX0NCj4gPiArDQo+ID4gKwlyZXR1cm4gcmV0Ow0KPiA+ICt9DQo+IENvdWxkIHRoaXMgYmUgcmVw
bGFjZWQgYnkgcnVudGltZSBwbT8NCltBbGlzb24gV2FuZ10gWWVzLCBJIHdpbGwgdXNlIHJ1bnRp
bWUgcG0uDQo+IFsuLi5dDQo+ID4gK3N0YXRpYyBpbnQgYnlwYXNzX3Rjb24oc3RydWN0IGRldmlj
ZV9ub2RlICpucCkgew0KPiA+ICsJc3RydWN0IGRldmljZV9ub2RlICp0Y29uX25wOw0KPiA+ICsJ
c3RydWN0IHBsYXRmb3JtX2RldmljZSAqdGNvbl9wZGV2Ow0KPiA+ICsJc3RydWN0IGNsayAqdGNv
bl9jbGs7DQo+ID4gKwl2b2lkIF9faW9tZW0gKnRjb25fcmVnOw0KPiA+ICsJaW50IHJldCA9IDA7
DQo+ID4gKw0KPiA+ICsJdGNvbl9ucCA9IG9mX3BhcnNlX3BoYW5kbGUobnAsICJ0Y29uLWNvbnRy
b2xsZXIiLCAwKTsNCj4gPiArCWlmICghdGNvbl9ucCkNCj4gPiArCQlyZXR1cm4gLUVJTlZBTDsN
Cj4gPiArDQo+ID4gKwl0Y29uX3BkZXYgPSBvZl9maW5kX2RldmljZV9ieV9ub2RlKHRjb25fbnAp
Ow0KPiA+ICsJaWYgKCF0Y29uX3BkZXYpDQo+ID4gKwkJcmV0dXJuIC1FSU5WQUw7DQo+ID4gKw0K
PiA+ICsJdGNvbl9jbGsgPSBkZXZtX2Nsa19nZXQoJnRjb25fcGRldi0+ZGV2LCAidGNvbiIpOw0K
PiA+ICsJaWYgKElTX0VSUih0Y29uX2NsaykpIHsNCj4gPiArCQlyZXQgPSBQVFJfRVJSKHRjb25f
Y2xrKTsNCj4gPiArCQlnb3RvIGZhaWxlZF9nZXRjbG9jazsNCj4gPiArCX0NCj4gPiArCWNsa19w
cmVwYXJlX2VuYWJsZSh0Y29uX2Nsayk7DQo+ID4gKw0KPiA+ICsJdGNvbl9yZWcgPSBvZl9pb21h
cCh0Y29uX25wLCAwKTsNCj4gPiArCWlmICghdGNvbl9yZWcpIHsNCj4gPiArCQlyZXQgPSAtRU5P
TUVNOw0KPiA+ICsJCWdvdG8gZmFpbGVkX2lvcmVtYXA7DQo+ID4gKwl9DQo+ID4gKwl3cml0ZWwo
VENPTl9CWVBBU1NfRU5BQkxFLCB0Y29uX3JlZyArIFRDT05fQ1RSTDEpOw0KPiA+ICsNCj4gPiAr
CXJldHVybiAwOw0KPiA+ICsNCj4gPiArZmFpbGVkX2lvcmVtYXA6DQo+ID4gKwljbGtfZGlzYWJs
ZV91bnByZXBhcmUodGNvbl9jbGspOw0KPiA+ICtmYWlsZWRfZ2V0Y2xvY2s6DQo+ID4gKwlvZl9u
b2RlX3B1dCh0Y29uX25wKTsNCj4gPiArCXJldHVybiByZXQ7DQo+ID4gK30NCj4gPiArDQo+IElz
IHRoZSBmcmFtZWJ1ZmZlciBkcml2ZXIgdGhlIG9ubHkgdXNlciBvZiB0Y29uPyBJZiBub3QgeW91
IHNob3VsZCBub3QNCj4gbWFwIHRoZSBtZW1vcnkgaGVyZSwgYnV0IHJhdGhlciB1c2Ugc29tZXRo
aW5nIGxpa2UgcmVnbWFwIHN5c2Nvbi4NCltBbGlzb24gV2FuZ10gWWVzLCB0aGUgZmIgZHJpdmVy
IGlzIHRoZSBvbmx5IHVzZXIgb2YgdGNvbi4NCg0KVGhhbmtzIGZvciB5b3VyIGNvbW1lbnRzIQ0K
DQoNCkJlc3QgUmVnYXJkcywNCkFsaXNvbiBXYW5nDQoNCg0KDQo
^ permalink raw reply
* Re: [RFC 0/1] drm/pl111: Initial drm/kms driver for pl111
From: Rob Clark @ 2013-08-07 13:27 UTC (permalink / raw)
To: John Stultz
Cc: Tom Cooksey, linux-fbdev, Pawel Moll, lkml, dri-devel,
linaro-mm-sig, linux-arm-kernel, linux-media,
Rebecca Schultz Zavin, Erik Gilling, Ross Oldfield
In-Reply-To: <CALAqxLW_rjS_bbDXDrPrnBRLbegs9TVmPDnpNVYuoQjaVv3tPw@mail.gmail.com>
On Wed, Aug 7, 2013 at 12:23 AM, John Stultz <john.stultz@linaro.org> wrote:
> On Tue, Aug 6, 2013 at 5:15 AM, Rob Clark <robdclark@gmail.com> wrote:
>> well, let's divide things up into two categories:
>>
>> 1) the arrangement and format of pixels.. ie. what userspace would
>> need to know if it mmap's a buffer. This includes pixel format,
>> stride, etc. This should be negotiated in userspace, it would be
>> crazy to try to do this in the kernel.
>>
>> 2) the physical placement of the pages. Ie. whether it is contiguous
>> or not. Which bank the pages in the buffer are placed in, etc. This
>> is not visible to userspace. This is the purpose of the attach step,
>> so you know all the devices involved in sharing up front before
>> allocating the backing pages. (Or in the worst case, if you have a
>> "late attacher" you at least know when no device is doing dma access
>> to a buffer and can reallocate and move the buffer.) A long time
>
> One concern I know the Android folks have expressed previously (and
> correct me if its no longer an objection), is that this attach time
> in-kernel constraint solving / moving or reallocating buffers is
> likely to hurt determinism. If I understood, their perspective was
> that userland knows the device path the buffers will travel through,
> so why not leverage that knowledge, rather then having the kernel have
> to sort it out for itself after the fact.
If you know the device path, then attach the buffer at all the devices
before you start using it. Problem solved.. kernel knows all devices
before pages need be allocated ;-)
BR,
-R
^ permalink raw reply
* Re: [RFC 1/1] drm/pl111: Initial drm/kms driver for pl111
From: Daniel Vetter @ 2013-08-07 16:46 UTC (permalink / raw)
To: Rob Clark; +Cc: dri-devel, linux-fbdev, pawel.moll, linux-arm-kernel
In-Reply-To: <CAF6AEGvGG1-4k-3_YHQ2ES6JEb-V-Xuicc8gfw9rPWze5JUEDg@mail.gmail.com>
Just comment a bit on Rob's review with my own opinion.
On Wed, Aug 07, 2013 at 12:17:21PM -0400, Rob Clark wrote:
> On Thu, Jul 25, 2013 at 1:17 PM, <tom.cooksey@arm.com> wrote:
> > From: Tom Cooksey <tom.cooksey@arm.com>
> >
> > This is a mode-setting driver for the pl111 CLCD display controller
> > found on various ARM reference platforms such as the Versatile
> > Express. The driver supports setting of a single mode (640x480) and
> > has only been tested on Versatile Express with a Cortex-A9 core tile.
> >
> > Known issues:
> > * It still includes code to use KDS, which is not going upstream.
>
> review's on http://lists.freedesktop.org/archives/dri-devel/2013-July/042462.html
> can't hurt
>
> although you might consider submitting a reduced functionality driver
> w/ KDS bits removed in the mean time.. then when the fence stuff is
> merged it is just an incremental patch rather than a whole driver ;-)
Yeah, I think the KDS bits and comments need to go first before merginge.
> > +/*
> > + * Number of flips allowed in flight at any one time. Any more flips requested
> > + * beyond this value will cause the caller to block until earlier flips have
> > + * completed.
> > + *
> > + * For performance reasons, this must be greater than the number of buffers
> > + * used in the rendering pipeline. Note that the rendering pipeline can contain
> > + * different types of buffer, e.g.:
> > + * - 2 final framebuffers
> > + * - >2 geometry buffers for GPU use-cases
> > + * - >2 vertex buffers for GPU use-cases
> > + *
> > + * For example, a system using 5 geometry buffers could have 5 flips in flight,
> > + * and so NR_FLIPS_IN_FLIGHT_THRESHOLD must be 5 or greater.
> > + *
> > + * Whilst there may be more intermediate buffers (such as vertex/geometry) than
> > + * final framebuffers, KDS is used to ensure that GPU rendering waits for the
> > + * next off-screen buffer, so it doesn't overwrite an on-screen buffer and
> > + * produce tearing.
> > + */
> > +
>
> fwiw, this is at least different from how other drivers do triple (or
> > double) buffering. In other drivers (intel, omap, and
> msm/freedreno, that I know of, maybe others too) the xorg driver dri2
> bits implement the double buffering (ie. send flip event back to
> client immediately and queue up the flip and call page-flip after the
> pageflip event back from kernel.
>
> I'm not saying not to do it this way, I guess I'd like to hear what
> other folks think. I kinda prefer doing this in userspace as it keeps
> the kernel bits simpler (plus it would then work properly on exynosdrm
> or other kms drivers).
Yeah, if this is just a sw queue then I don't think it makes sense to have
it in the kernel. Afaik the current pageflip interface drm exposes allows
one oustanding flip only, and you _must_ wait for the flip complete event
before you can submit the second one.
Ofc if your hardware as a hw-based flip queue (maybe even with frame
targets) that's a different matter, but currently we don't have a drm
interface to expose this. I'd say for merging the basic driver first we
should go with the existing simple pageflip semantics.
And tbh I don't understand why the amount of buffers you keep in the
render pipeline side of things matters here at all. But I also haven't
read the details of your driver code.
>
> > +/*
> > + * Here, we choose a conservative value. A lower value is most likely
> > + * suitable for GPU use-cases.
> > + */
> > +#define NR_FLIPS_IN_FLIGHT_THRESHOLD 16
> > +
> > +#define CLCD_IRQ_NEXTBASE_UPDATE (1u<<2)
> > +
> > +struct pl111_drm_flip_resource;
> > +struct pl111_drm_cursor_plane;
> > +
> > +enum pl111_bo_type {
> > + PL111_BOT_DMA,
> > + PL111_BOT_SHM
> > +};
> > +
> > +struct pl111_gem_bo_dma {
> > + dma_addr_t fb_dev_addr;
> > + void *fb_cpu_addr;
> > +};
> > +
> > +struct pl111_gem_bo_shm {
> > + struct page **pages;
> > + dma_addr_t *dma_addrs;
> > +};
> > +
> > +struct pl111_gem_bo {
> > + struct drm_gem_object gem_object;
> > + enum pl111_bo_type type;
> > + union {
> > + struct pl111_gem_bo_dma dma;
> > + struct pl111_gem_bo_shm shm;
> > + } backing_data;
> > + struct drm_framebuffer *fb;
>
> this is at least a bit odd.. normally the fb has ref to the bo(s) and
> not the other way around. And the same bo could be referenced by
> multiple fb's which would kinda fall down with this approach.
I'd say that's just backwards, framebuffers are created from backing
storage objects (which for a gem based driver is a gem object), not the
other way round. What's this exactly used for?
[snip]
> > +
> > + /*
> > + * Used to prevent race between pl111_dma_buf_release and
> > + * drm_gem_prime_handle_to_fd
> > + */
> > + struct mutex export_dma_buf_lock;
>
> hmm, seems a bit suspicious.. the handle reference should keep the
> object live. Ie. either drm_gem_object_lookup() will fail because the
> object is gone (userspace has closed it's handle ref and
> dmabuf->release() already dropped it's ref) or it will succeed and
> you'll have a reference to the bo keeping it from going away if the
> release() comes after.
The race is real, I have an evil testcase here which Oopses my kernel. I'm
working on a fix (v1 of my patches is submitted a few weeks back, awaiting
review), but I need to rework a few things since now I've also spotted a
leak or two ;-)
[snip]
> > +static void vsync_worker(struct work_struct *work)
> > +{
> > + struct pl111_drm_flip_resource *flip_res;
> > + struct pl111_gem_bo *bo;
> > + struct pl111_drm_crtc *pl111_crtc;
> > + struct drm_device *dev;
> > + int flips_in_flight;
> > + flip_res > > + container_of(work, struct pl111_drm_flip_resource, vsync_work);
> > +
> > + pl111_crtc = to_pl111_crtc(flip_res->crtc);
> > + dev = pl111_crtc->crtc.dev;
> > +
> > + DRM_DEBUG_KMS("DRM Finalizing flip_res=%p\n", flip_res);
> > +
> > + bo = PL111_BO_FROM_FRAMEBUFFER(flip_res->fb);
> > +#ifdef CONFIG_DMA_SHARED_BUFFER_USES_KDS
> > + if (flip_res->worker_release_kds = true) {
> > + spin_lock(&pl111_crtc->current_displaying_lock);
> > + release_kds_resource_and_display(flip_res);
> > + spin_unlock(&pl111_crtc->current_displaying_lock);
> > + }
> > +#endif
> > + /* Release DMA buffer on this flip */
> > + if (bo->gem_object.export_dma_buf != NULL)
> > + dma_buf_put(bo->gem_object.export_dma_buf);
>
> I think you just want to unref the outgoing bo, and let it drop the
> dmabuf ref when the file ref of the imported bo goes. Or actually, it
> would be better to hold/drop ref's to the fb, rather than the bo. At
> least this will make things simpler if you ever have multi-planar
> support.
Drivers have no business frobbing around the dma-buf refcount of imported
objects imo, at least if they use all the standard drm prime
infrastructure. And if they're bugs they need to be fixed there, not in
drivers.
[snip]
> > +struct drm_framebuffer *pl111_fb_create(struct drm_device *dev,
> > + struct drm_file *file_priv,
> > + struct drm_mode_fb_cmd2 *mode_cmd)
> > +{
> > + struct pl111_drm_framebuffer *pl111_fb = NULL;
> > + struct drm_framebuffer *fb = NULL;
> > + struct drm_gem_object *gem_obj;
> > + struct pl111_gem_bo *bo;
> > +
> > + pr_info("DRM %s\n", __func__);
> > + gem_obj = drm_gem_object_lookup(dev, file_priv, mode_cmd->handles[0]);
> > + if (gem_obj = NULL) {
> > + DRM_ERROR("Could not get gem obj from handle to create fb\n");
> > + goto out;
> > + }
> > +
> > + bo = PL111_BO_FROM_GEM(gem_obj);
> > + /* Don't even attempt PL111_BOT_SHM, it's not contiguous */
> > + BUG_ON(bo->type != PL111_BOT_DMA);
>
> umm, no BUG_ON() is not really a good way to validate userspace input..
>
> if (bo->type != ...)
> return ERR_PTR(-EINVAL);
Yep.
> > +
> > + switch ((char)(mode_cmd->pixel_format & 0xFF)) {
> > + case 'Y':
> > + case 'U':
> > + case 'V':
> > + case 'N':
> > + case 'T':
>
> perhaps we should instead add a drm_format_is_yuv().. or you could
> (ab)use drm_fb_get_bpp_depth()..
Yeah, I think a new drm_format_is_yuv is asked-for here. Now the bigger
question is why you need this, since the drm core should filter out
formats not in your list of supported ones. Or at least it should ...
Cheers, Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
^ permalink raw reply
* Re: [Linaro-mm-sig] [RFC 0/1] drm/pl111: Initial drm/kms driver for pl111
From: Alex Deucher @ 2013-08-07 17:56 UTC (permalink / raw)
To: Tom Cooksey
Cc: Rob Clark, linux-fbdev, Pawel Moll, linux-kernel, dri-devel,
linaro-mm-sig, linux-arm-kernel, linux-media
In-Reply-To: <520284fe.a16ec20a.2d3c.6e19SMTPIN_ADDED_BROKEN@mx.google.com>
On Wed, Aug 7, 2013 at 1:33 PM, Tom Cooksey <tom.cooksey@arm.com> wrote:
>
>> >> > Didn't you say that programmatically describing device placement
>> >> > constraints was an unbounded problem? I guess we would have to
>> >> > accept that it's not possible to describe all possible constraints
>> >> > and instead find a way to describe the common ones?
>> >>
>> >> well, the point I'm trying to make, is by dividing your constraints
>> >> into two groups, one that impacts and is handled by userspace, and
>> >> one that is in the kernel (ie. where the pages go), you cut down
>> >> the number of permutations that the kernel has to care about
>> >> considerably. And kernel already cares about, for example, what
>> >> range of addresses that a device can dma to/from. I think really
>> >> the only thing missing is the max # of sglist entries (contiguous
>> >> or not)
>> >
>> > I think it's more than physically contiguous or not.
>> >
>> > For example, it can be more efficient to use large page sizes on
>> > devices with IOMMUs to reduce TLB traffic. I think the size and even
>> > the availability of large pages varies between different IOMMUs.
>>
>> sure.. but I suppose if we can spiff out dma_params to express "I need
>> contiguous", perhaps we can add some way to express "I prefer
>> as-contiguous-as-possible".. either way, this is about where the pages
>> are placed, and not about the layout of pixels within the page, so
>> should be in kernel. It's something that is missing, but I believe
>> that it belongs in dma_params and hidden behind dma_alloc_*() for
>> simple drivers.
>
> Thinking about it, isn't this more a property of the IOMMU? I mean,
> are there any cases where an IOMMU had a large page mode but you
> wouldn't want to use it? So when allocating the memory, you'd have to
> take into account not just the constraints of the devices themselves,
> but also of any IOMMUs any of the device sit behind?
>
>
>> > There's also the issue of buffer stride alignment. As I say, if the
>> > buffer is to be written by a tile-based GPU like Mali, it's more
>> > efficient if the buffer's stride is aligned to the max AXI bus burst
>> > length. Though I guess a buffer stride only makes sense as a concept
>> > when interpreting the data as a linear-layout 2D image, so perhaps
>> > belongs in user-space along with format negotiation?
>> >
>>
>> Yeah.. this isn't about where the pages go, but about the arrangement
>> within a page.
>>
>> And, well, except for hw that supports the same tiling (or
>> compressed-fb) in display+gpu, you probably aren't sharing tiled
>> buffers.
>
> You'd only want to share a buffer between devices if those devices can
> understand the same pixel format. That pixel format can't be device-
> specific or opaque, it has to be explicit. I think drm_fourcc.h is
> what defines all the possible pixel formats. This is the enum I used
> in EGL_EXT_image_dma_buf_import at least. So if we get to the point
> where multiple devices can understand a tiled or compressed format, I
> assume we could just add that format to drm_fourcc.h and possibly
> v4l2's v4l2_mbus_pixelcode enum in v4l2-mediabus.h.
>
> For user-space to negotiate a common pixel format and now stride
> alignment, I guess it will obviously need a way to query what pixel
> formats a device supports and what its stride alignment requirements
> are.
>
> I don't know v4l2 very well, but it certainly seems the pixel format
> can be queried using V4L2_SUBDEV_FORMAT_TRY when attempting to set
> a particular format. I couldn't however find a way to retrieve a list
> of supported formats - it seems the mechanism is to try out each
> format in turn to determine if it is supported. Is that right?
>
> There doesn't however seem a way to query what stride constraints a
> V4l2 device might have. Does HW abstracted by v4l2 typically have
> such constraints? If so, how can we query them such that a buffer
> allocated by a DRM driver can be imported into v4l2 and used with
> that HW?
>
> Turning to DRM/KMS, it seems the supported formats of a plane can be
> queried using drm_mode_get_plane. However, there doesn't seem to be a
> way to query the supported formats of a crtc? If display HW only
> supports scanning out from a single buffer (like pl111 does), I think
> it won't have any planes and a fb can only be set on the crtc. In
> which case, how should user-space query which pixel formats that crtc
> supports?
>
> Assuming user-space can query the supported formats and find a common
> one, it will need to allocate a buffer. Looks like
> drm_mode_create_dumb can do that, but it only takes a bpp parameter,
> there's no format parameter. I assume then that user-space defines
> the format and tells the DRM driver which format the buffer is in
> when creating the fb with drm_mode_fb_cmd2, which does take a format
> parameter? Is that right?
>
> As with v4l2, DRM doesn't appear to have a way to query the stride
> constraints? Assuming there is a way to query the stride constraints,
> there also isn't a way to specify them when creating a buffer with
> DRM, though perhaps the existing pitch parameter of
> drm_mode_create_dumb could be used to allow user-space to pass in a
> minimum stride as well as receive the allocated stride?
>
>
>> >> > One problem with this is it duplicates a lot of logic in each
>> >> > driver which can export a dma_buf buffer. Each exporter will
>> >> > need to do pretty much the same thing: iterate over all the
>> >> > attachments, determine of all the constraints (assuming that
>> >> > can be done) and allocate pages such that the lowest-common-
>> >> > denominator is satisfied. Perhaps rather than duplicating that
>> >> > logic in every driver, we could instead move allocation of the
>> >> > backing pages into dma_buf itself?
>> >>
>> >> I tend to think it is better to add helpers as we see common
>>
>> >> patterns emerge, which drivers can opt-in to using. I don't
>> >> think that we should move allocation into dma_buf itself, but
>> >> it would perhaps be useful to have dma_alloc_*() variants that
>> >> could allocate for multiple devices.
>> >
>> > A helper could work I guess, though I quite like the idea of
>> > having dma_alloc_*() variants which take a list of devices to
>> > allocate memory for.
>> >
>> >
>> >> That would help for simple stuff, although I'd suspect
>> >> eventually a GPU driver will move away from that. (Since you
>> >> probably want to play tricks w/ pools of pages that are
>> >> pre-zero'd and in the correct cache state, use spare cycles on
>> >> the gpu or dma engine to pre-zero uncached pages, and games
>> >> like that.)
>> >
>> > So presumably you're talking about a GPU driver being the exporter
>> > here? If so, how could the GPU driver do these kind of tricks on
>> > memory shared with another device?
>>
>> Yes, that is gpu-as-exporter. If someone else is allocating buffers,
>> it is up to them to do these tricks or not. Probably there is a
>> pretty good chance that if you aren't a GPU you don't need those sort
>> of tricks for fast allocation of transient upload buffers, staging
>> textures, temporary pixmaps, etc. Ie. I don't really think a v4l
>> camera or video decoder would benefit from that sort of optimization.
>
> Right - but none of those are really buffers you'd want to export with
> dma_buf to share with another device are they? In which case, why not
> just have dma_buf figure out the constraints and allocate the memory?
>
> If a driver needs to allocate memory in a special way for a particular
> device, I can't really imagine how it would be able to share that
> buffer with another device using dma_buf? I guess a driver is likely
> to need some magic voodoo to configure access to the buffer for its
> device, but surely that would be done by the dma_mapping framework
> when dma_buf_map happens?
>
>
>
>> >> You probably want to get out of the SoC mindset, otherwise you are
>> >> going to make bad assumptions that come back to bite you later on.
>> >
>> > Sure - there are always going to be PC-like devices where the
>> > hardware configuration isn't fixed like it is on a traditional SoC.
>> > But I'd rather have a simple solution which works on traditional SoCs
>> > than no solution at all. Today our solution is to over-load the dumb
>> > buffer alloc functions of the display's DRM driver - For now I'm just
>> > looking for the next step up from that! ;-)
>>
>> True.. the original intention, which is perhaps a bit desktop-centric,
>> really was for there to be a userspace component talking to the drm
>> driver for allocation, ie. xf86-video-foo and/or
>> src/gallium/drivers/foo (for example) ;-)
>>
>> Which means for x11 having a SoC vendor specific xf86-video-foo for
>> x11.. or vendor specific gbm implementation for wayland. (Although
>> at least in the latter case it is a pretty small piece of code.) But
>> that is probably what you are trying to avoid.
>
> I've been trying to get my head around how PRIME relates to DDX
> drivers. As I understand it (which is likely wrong), you have a laptop
> with both an Intel & an nVidia GPU. You have both the i915 & nouveau
> kernel drivers loaded. What I'm not sure about is which GPU's display
> controller is actually hooked up to the physical connector? Perhaps
> there is a MUX like there is on Versatile Express?
>
> What I also don't understand is what DDX driver is loaded? Is it
> xf86-video-intel, xf86-video-nouveau or both? I get the impression
> that there's a "master" DDX which implements 2D operations but can
> import buffers using PRIME from the other driver and draw to them.
> Or is it more that it's able to export rendered buffers to the
> "slave" DRM for scanout? Either way, it's pretty similar to an ARM
> SoC setup which has the GPU and the display as two totally
> independent devices.
>
In the early days, there was a MUX to switch the displays between the
two GPUs, but most modern systems are MUX-less and the dGPU is either
connected to no displays or in some cases the local panel is attached
to the integrated GPU and the external displays are connected to the
dGPU.
In the MUX-less case, the dGPU can be used to render, and then the
result is copied to the integrated GPU for display.
Alex
^ permalink raw reply
* Re: [RFC 0/1] drm/pl111: Initial drm/kms driver for pl111
From: Rob Clark @ 2013-08-07 18:12 UTC (permalink / raw)
To: Tom Cooksey
Cc: dri-devel, linux-fbdev, Pawel Moll, linux-arm-kernel, linux-media,
linaro-mm-sig, linux-kernel
In-Reply-To: <520284d6.07300f0a.72a4.1623SMTPIN_ADDED_BROKEN@mx.google.com>
On Wed, Aug 7, 2013 at 1:33 PM, Tom Cooksey <tom.cooksey@arm.com> wrote:
>
>> >> > Didn't you say that programmatically describing device placement
>> >> > constraints was an unbounded problem? I guess we would have to
>> >> > accept that it's not possible to describe all possible constraints
>> >> > and instead find a way to describe the common ones?
>> >>
>> >> well, the point I'm trying to make, is by dividing your constraints
>> >> into two groups, one that impacts and is handled by userspace, and
>> >> one that is in the kernel (ie. where the pages go), you cut down
>> >> the number of permutations that the kernel has to care about
>> >> considerably. And kernel already cares about, for example, what
>> >> range of addresses that a device can dma to/from. I think really
>> >> the only thing missing is the max # of sglist entries (contiguous
>> >> or not)
>> >
>> > I think it's more than physically contiguous or not.
>> >
>> > For example, it can be more efficient to use large page sizes on
>> > devices with IOMMUs to reduce TLB traffic. I think the size and even
>> > the availability of large pages varies between different IOMMUs.
>>
>> sure.. but I suppose if we can spiff out dma_params to express "I need
>> contiguous", perhaps we can add some way to express "I prefer
>> as-contiguous-as-possible".. either way, this is about where the pages
>> are placed, and not about the layout of pixels within the page, so
>> should be in kernel. It's something that is missing, but I believe
>> that it belongs in dma_params and hidden behind dma_alloc_*() for
>> simple drivers.
>
> Thinking about it, isn't this more a property of the IOMMU? I mean,
> are there any cases where an IOMMU had a large page mode but you
> wouldn't want to use it? So when allocating the memory, you'd have to
> take into account not just the constraints of the devices themselves,
> but also of any IOMMUs any of the device sit behind?
>
perhaps yes. But the device is associated w/ the iommu it is attached
to, so this shouldn't be a problem
>
>> > There's also the issue of buffer stride alignment. As I say, if the
>> > buffer is to be written by a tile-based GPU like Mali, it's more
>> > efficient if the buffer's stride is aligned to the max AXI bus burst
>> > length. Though I guess a buffer stride only makes sense as a concept
>> > when interpreting the data as a linear-layout 2D image, so perhaps
>> > belongs in user-space along with format negotiation?
>> >
>>
>> Yeah.. this isn't about where the pages go, but about the arrangement
>> within a page.
>>
>> And, well, except for hw that supports the same tiling (or
>> compressed-fb) in display+gpu, you probably aren't sharing tiled
>> buffers.
>
> You'd only want to share a buffer between devices if those devices can
> understand the same pixel format. That pixel format can't be device-
> specific or opaque, it has to be explicit. I think drm_fourcc.h is
> what defines all the possible pixel formats. This is the enum I used
> in EGL_EXT_image_dma_buf_import at least. So if we get to the point
> where multiple devices can understand a tiled or compressed format, I
> assume we could just add that format to drm_fourcc.h and possibly
> v4l2's v4l2_mbus_pixelcode enum in v4l2-mediabus.h.
>
> For user-space to negotiate a common pixel format and now stride
> alignment, I guess it will obviously need a way to query what pixel
> formats a device supports and what its stride alignment requirements
> are.
>
> I don't know v4l2 very well, but it certainly seems the pixel format
> can be queried using V4L2_SUBDEV_FORMAT_TRY when attempting to set
> a particular format. I couldn't however find a way to retrieve a list
> of supported formats - it seems the mechanism is to try out each
> format in turn to determine if it is supported. Is that right?
it is exposed for drm plane's. What is missing is to expose the
primary-plane associated with the crtc.
> There doesn't however seem a way to query what stride constraints a
> V4l2 device might have. Does HW abstracted by v4l2 typically have
> such constraints? If so, how can we query them such that a buffer
> allocated by a DRM driver can be imported into v4l2 and used with
> that HW?
>
> Turning to DRM/KMS, it seems the supported formats of a plane can be
> queried using drm_mode_get_plane. However, there doesn't seem to be a
> way to query the supported formats of a crtc? If display HW only
> supports scanning out from a single buffer (like pl111 does), I think
> it won't have any planes and a fb can only be set on the crtc. In
> which case, how should user-space query which pixel formats that crtc
> supports?
>
> Assuming user-space can query the supported formats and find a common
> one, it will need to allocate a buffer. Looks like
> drm_mode_create_dumb can do that, but it only takes a bpp parameter,
> there's no format parameter. I assume then that user-space defines
> the format and tells the DRM driver which format the buffer is in
> when creating the fb with drm_mode_fb_cmd2, which does take a format
> parameter? Is that right?
Right, the gem object has no inherent format, it is just some bytes.
The format/width/height/pitch are all attributes of the fb.
> As with v4l2, DRM doesn't appear to have a way to query the stride
> constraints? Assuming there is a way to query the stride constraints,
> there also isn't a way to specify them when creating a buffer with
> DRM, though perhaps the existing pitch parameter of
> drm_mode_create_dumb could be used to allow user-space to pass in a
> minimum stride as well as receive the allocated stride?
>
well, you really shouldn't be using create_dumb.. you should have a
userspace piece that is specific to the drm driver, and knows how to
use that driver's gem allocate ioctl.
>
>> >> > One problem with this is it duplicates a lot of logic in each
>> >> > driver which can export a dma_buf buffer. Each exporter will
>> >> > need to do pretty much the same thing: iterate over all the
>> >> > attachments, determine of all the constraints (assuming that
>> >> > can be done) and allocate pages such that the lowest-common-
>> >> > denominator is satisfied. Perhaps rather than duplicating that
>> >> > logic in every driver, we could instead move allocation of the
>> >> > backing pages into dma_buf itself?
>> >>
>> >> I tend to think it is better to add helpers as we see common
>>
>> >> patterns emerge, which drivers can opt-in to using. I don't
>> >> think that we should move allocation into dma_buf itself, but
>> >> it would perhaps be useful to have dma_alloc_*() variants that
>> >> could allocate for multiple devices.
>> >
>> > A helper could work I guess, though I quite like the idea of
>> > having dma_alloc_*() variants which take a list of devices to
>> > allocate memory for.
>> >
>> >
>> >> That would help for simple stuff, although I'd suspect
>> >> eventually a GPU driver will move away from that. (Since you
>> >> probably want to play tricks w/ pools of pages that are
>> >> pre-zero'd and in the correct cache state, use spare cycles on
>> >> the gpu or dma engine to pre-zero uncached pages, and games
>> >> like that.)
>> >
>> > So presumably you're talking about a GPU driver being the exporter
>> > here? If so, how could the GPU driver do these kind of tricks on
>> > memory shared with another device?
>>
>> Yes, that is gpu-as-exporter. If someone else is allocating buffers,
>> it is up to them to do these tricks or not. Probably there is a
>> pretty good chance that if you aren't a GPU you don't need those sort
>> of tricks for fast allocation of transient upload buffers, staging
>> textures, temporary pixmaps, etc. Ie. I don't really think a v4l
>> camera or video decoder would benefit from that sort of optimization.
>
> Right - but none of those are really buffers you'd want to export with
> dma_buf to share with another device are they? In which case, why not
> just have dma_buf figure out the constraints and allocate the memory?
maybe not.. but (a) you don't necessarily know at creation time if it
is going to be exported (maybe you know if it is definitely not going
to be exported, but the converse is not true), and (b) there isn't
really any reason to special case the allocation in the driver because
it is going to be exported.
helpers that can be used by simple drivers, yes. Forcing the way the
buffer is allocated, for sure not. Currently, for example, there is
no issue to export a buffer allocated from stolen-mem. If we put the
page allocation in dma-buf, this would not be possible. That is just
one quick example off the top of my head, I'm sure there are plenty
more. But we definitely do not want the allocate in dma_buf itself.
> If a driver needs to allocate memory in a special way for a particular
> device, I can't really imagine how it would be able to share that
> buffer with another device using dma_buf? I guess a driver is likely
> to need some magic voodoo to configure access to the buffer for its
> device, but surely that would be done by the dma_mapping framework
> when dma_buf_map happens?
>
if, what it has to configure actually manages to fit in the
dma-mapping framework
anyways, where the pages come from has nothing to do with whether a
buffer can be shared or not
>
>
>> >> You probably want to get out of the SoC mindset, otherwise you are
>> >> going to make bad assumptions that come back to bite you later on.
>> >
>> > Sure - there are always going to be PC-like devices where the
>> > hardware configuration isn't fixed like it is on a traditional SoC.
>> > But I'd rather have a simple solution which works on traditional SoCs
>> > than no solution at all. Today our solution is to over-load the dumb
>> > buffer alloc functions of the display's DRM driver - For now I'm just
>> > looking for the next step up from that! ;-)
>>
>> True.. the original intention, which is perhaps a bit desktop-centric,
>> really was for there to be a userspace component talking to the drm
>> driver for allocation, ie. xf86-video-foo and/or
>> src/gallium/drivers/foo (for example) ;-)
>>
>> Which means for x11 having a SoC vendor specific xf86-video-foo for
>> x11.. or vendor specific gbm implementation for wayland. (Although
>> at least in the latter case it is a pretty small piece of code.) But
>> that is probably what you are trying to avoid.
>
> I've been trying to get my head around how PRIME relates to DDX
> drivers. As I understand it (which is likely wrong), you have a laptop
> with both an Intel & an nVidia GPU. You have both the i915 & nouveau
> kernel drivers loaded. What I'm not sure about is which GPU's display
> controller is actually hooked up to the physical connector? Perhaps
> there is a MUX like there is on Versatile Express?
afaiu it can be a, b, or c (ie. either gpu can have the display or
there can be a mux)..
> What I also don't understand is what DDX driver is loaded? Is it
> xf86-video-intel, xf86-video-nouveau or both? I get the impression
> that there's a "master" DDX which implements 2D operations but can
> import buffers using PRIME from the other driver and draw to them.
> Or is it more that it's able to export rendered buffers to the
> "slave" DRM for scanout? Either way, it's pretty similar to an ARM
> SoC setup which has the GPU and the display as two totally
> independent devices.
>
>
>
>> At any rate, for both xorg and wayland/gbm, you know when a buffer is
>> going to be a scanout buffer. What I'd recommend is define a small
>> userspace API that your customers (the SoC vendors) implement to
>> allocate a scanout buffer and hand you back a dmabuf fd. That could
>> be used both for x11 and for gbm. Inputs should be requested
>> width/height and format. And outputs pitch plus dmabuf fd.
>>
>> (Actually you might even just want to use gbm as your starting point.
>> You could probably just use gbm from xf86-video-armsoc for allocation,
>> to have one thing that works for both wayland and x11. Scanout and
>> cursor buffers should go to vendor/SoC specific fxn, rest can be
>> allocated from mali kernel driver.)
>
> What does that buy us over just using drm_mode_create_dumb on the
> display's DRM driver?
>
well, for example, if there was actually some hw w/ omap's dss + mali,
you could actually have mali render transparently to tiled buffers
which could be scanned out rotated. Which would not be possible w/
dumb buffers.
>
>
>> >> > wouldn't need a way to programmatically describe the constraints
>> >> > either: As you say, if userspace sets the "SCANOUT" flag, it would
>> >> > just "know" that on this SoC, that buffer needs to be physically
>> >> > contiguous for example.
>> >>
>> >> not really.. it just knows it wants to scanout the buffer, and tells
>> >> this as a hint to the kernel.
>> >>
>> >> For example, on omapdrm, the SCANOUT flag does nothing on omap4+
>> >> (where phys contig is not required for scanout), but causes CMA
>> >> (dma_alloc_*()) to be used on omap3. Userspace doesn't care.
>> >> It just knows that it wants to be able to scanout that particular
>> >> buffer.
>> >
>> > I think that's the idea? The omap3's allocator driver would use
>> > contiguous memory when it detects the SCANOUT flag whereas the omap4
>> > allocator driver wouldn't have to. No complex negotiation of
>> > constraints - it just "knows".
>> >
>>
>> well, it is same allocating driver in both cases (although maybe that
>> is unimportant). The "it" that just knows it wants to scanout is
>> userspace. The "it" that just knows that scanout translates to
>> contiguous (or not) is the kernel. Perhaps we are saying the same
>> thing ;-)
>
> Yeah - I think we are... so what's the issue with having a per-SoC
> allocation driver again?
>
In a way the display driver is a per-SoC allocator. But not
necessarily the *central* allocator for everything. Ie. no need for
display driver to allocate vertex buffers for a separate gpu driver,
and that sort of thing.
BR,
-R
>
>
> Cheers,
>
> Tom
>
>
>
>
>
^ permalink raw reply
* [RFC 0/1] Adding DT support to video/da8xx-fb.c
From: Darren Etheridge @ 2013-08-08 20:15 UTC (permalink / raw)
To: devicetree, tomi.valkeinen, plagnioj, linux-fbdev, detheridge; +Cc: afzal
This is part of a larger series of patches to upgrade the da8xx-fb.c driver
to support the Texas Instruments AM335x device. As part of this upgrade
we also want to add devicetree support for both the original da8xx and the
am335x. Tomi Valkeinen has reviewed the fbdev changes but he suggested
that it was prudent to extract the dt pieces and run it through the
devicetree mailing list for review.
Thanks,
Darren
Darren Etheridge (1):
video: da8xx-fb: adding dt support
.../devicetree/bindings/video/fb-da8xx.txt | 37 +++++++++++
drivers/video/da8xx-fb.c | 67 +++++++++++++++++++-
2 files changed, 101 insertions(+), 3 deletions(-)
create mode 100644 Documentation/devicetree/bindings/video/fb-da8xx.txt
^ permalink raw reply
* [RFC 1/1] video: da8xx-fb: adding dt support
From: Darren Etheridge @ 2013-08-08 20:15 UTC (permalink / raw)
To: devicetree, tomi.valkeinen, plagnioj, linux-fbdev, detheridge; +Cc: afzal
In-Reply-To: <1375992936-16755-1-git-send-email-detheridge@ti.com>
Enhancing driver to enable probe triggered by a corresponding dt entry.
Add da8xx-fb.txt documentation to devicetree section.
Obtain fb_videomode details for the connected lcd panel using the
display timing details present in DT.
Ensure that platform data is present before checking whether platform
callback is present (the one used to control backlight). So far this
was not an issue as driver was purely non-DT triggered, but now DT
support has been added this check must be performed.
v2: squashing multiple commits from Afzal Mohammed (afzal@ti.com)
v3: remove superfluous cast
v4: expose both ti,am3352-lcdc and ti,da830-lcdc for .compatible
as driver can use enhanced features of all version of the
silicon block.
Signed-off-by: Darren Etheridge <detheridge@ti.com>
---
.../devicetree/bindings/video/fb-da8xx.txt | 37 +++++++++++
drivers/video/da8xx-fb.c | 67 +++++++++++++++++++-
2 files changed, 101 insertions(+), 3 deletions(-)
create mode 100644 Documentation/devicetree/bindings/video/fb-da8xx.txt
diff --git a/Documentation/devicetree/bindings/video/fb-da8xx.txt b/Documentation/devicetree/bindings/video/fb-da8xx.txt
new file mode 100644
index 0000000..a6cfe3c
--- /dev/null
+++ b/Documentation/devicetree/bindings/video/fb-da8xx.txt
@@ -0,0 +1,37 @@
+TI LCD Controller on DA830/DA850/AM335x SoC's
+
+Required properties:
+- compatible:
+ DA830 - "ti,da830-lcdc"
+ AM335x SoC's - "ti,am3352-lcdc"
+- reg: Address range of lcdc register set
+- interrupts: lcdc interrupt
+- display-timings: typical videomode of lcd panel, represented as child.
+ Refer Documentation/devicetree/bindings/video/display-timing.txt for
+ display timing binding details. If multiple videomodes are mentioned
+ in display timings node, typical videomode has to be mentioned as the
+ native mode or it has to be first child (driver cares only for native
+ videomode).
+
+Example:
+
+lcdc@4830e000 {
+ compatible = "ti,am3352-lcdc";
+ reg = <0x4830e000 0x1000>;
+ interrupts = <36>;
+ display-timings {
+ 800x480p62 {
+ clock-frequency = <30000000>;
+ hactive = <800>;
+ vactive = <480>;
+ hfront-porch = <39>;
+ hback-porch = <39>;
+ hsync-len = <47>;
+ vback-porch = <29>;
+ vfront-porch = <13>;
+ vsync-len = <2>;
+ hsync-active = <1>;
+ vsync-active = <1>;
+ };
+ };
+};
diff --git a/drivers/video/da8xx-fb.c b/drivers/video/da8xx-fb.c
index ea9771f..64fd8af 100644
--- a/drivers/video/da8xx-fb.c
+++ b/drivers/video/da8xx-fb.c
@@ -36,6 +36,7 @@
#include <linux/slab.h>
#include <linux/delay.h>
#include <linux/lcm.h>
+#include <video/of_display_timing.h>
#include <video/da8xx-fb.h>
#include <asm/div64.h>
@@ -1297,12 +1298,57 @@ static struct fb_ops da8xx_fb_ops = {
.fb_blank = cfb_blank,
};
+static struct lcd_ctrl_config *da8xx_fb_create_cfg(struct platform_device *dev)
+{
+ struct lcd_ctrl_config *cfg;
+
+ cfg = devm_kzalloc(&dev->dev, sizeof(struct fb_videomode), GFP_KERNEL);
+ if (!cfg) {
+ dev_err(&dev->dev, "memory allocation failed\n");
+ return NULL;
+ }
+
+ /* default values */
+
+ if (lcd_revision = LCD_VERSION_1)
+ cfg->bpp = 16;
+ else
+ cfg->bpp = 32;
+
+ /*
+ * For panels so far used with this LCDC, below statement is sufficient.
+ * For new panels, if required, struct lcd_ctrl_cfg fields to be updated
+ * with additional/modified values. Those values would have to be then
+ * obtained from dt(requiring new dt bindings).
+ */
+
+ cfg->panel_shade = COLOR_ACTIVE;
+
+ return cfg;
+}
+
static struct fb_videomode *da8xx_fb_get_videomode(struct platform_device *dev)
{
struct da8xx_lcdc_platform_data *fb_pdata = dev->dev.platform_data;
struct fb_videomode *lcdc_info;
+ struct device_node *np = dev->dev.of_node;
int i;
+ if (np) {
+ lcdc_info = devm_kzalloc(&dev->dev,
+ sizeof(struct fb_videomode),
+ GFP_KERNEL);
+ if (!lcdc_info) {
+ dev_err(&dev->dev, "memory allocation failed\n");
+ return NULL;
+ }
+ if (of_get_fb_videomode(np, lcdc_info, OF_USE_NATIVE_MODE)) {
+ dev_err(&dev->dev, "timings not available in DT\n");
+ return NULL;
+ }
+ return lcdc_info;
+ }
+
for (i = 0, lcdc_info = known_lcd_panels;
i < ARRAY_SIZE(known_lcd_panels); i++, lcdc_info++) {
if (strcmp(fb_pdata->type, lcdc_info->name) = 0)
@@ -1331,7 +1377,7 @@ static int fb_probe(struct platform_device *device)
int ret;
unsigned long ulcm;
- if (fb_pdata = NULL) {
+ if (fb_pdata = NULL && !device->dev.of_node) {
dev_err(&device->dev, "Can not get platform data\n");
return -ENOENT;
}
@@ -1371,7 +1417,10 @@ static int fb_probe(struct platform_device *device)
break;
}
- lcd_cfg = (struct lcd_ctrl_config *)fb_pdata->controller_data;
+ if (device->dev.of_node)
+ lcd_cfg = da8xx_fb_create_cfg(device);
+ else
+ lcd_cfg = fb_pdata->controller_data;
if (!lcd_cfg) {
ret = -EINVAL;
@@ -1390,7 +1439,7 @@ static int fb_probe(struct platform_device *device)
par->dev = &device->dev;
par->lcdc_clk = tmp_lcdc_clk;
par->lcdc_clk_rate = clk_get_rate(par->lcdc_clk);
- if (fb_pdata->panel_power_ctrl) {
+ if (fb_pdata && fb_pdata->panel_power_ctrl) {
par->panel_power_ctrl = fb_pdata->panel_power_ctrl;
par->panel_power_ctrl(1);
}
@@ -1638,6 +1687,17 @@ static int fb_resume(struct platform_device *dev)
#define fb_resume NULL
#endif
+static const struct of_device_id da8xx_fb_of_match[] = {
+ /*
+ * this driver supports version 1 and version 2 of the
+ * Texas Instruments lcd controller (lcdc) hardware block
+ */
+ {.compatible = "ti,da830-lcdc", },
+ {.compatible = "ti,am3352-lcdc", },
+ {},
+};
+MODULE_DEVICE_TABLE(of, da8xx_fb_of_match);
+
static struct platform_driver da8xx_fb_driver = {
.probe = fb_probe,
.remove = fb_remove,
@@ -1646,6 +1706,7 @@ static struct platform_driver da8xx_fb_driver = {
.driver = {
.name = DRIVER_NAME,
.owner = THIS_MODULE,
+ .of_match_table = of_match_ptr(da8xx_fb_of_match),
},
};
--
1.7.0.4
^ permalink raw reply related
* Re: [RFC 1/1] video: da8xx-fb: adding dt support
From: Prabhakar Lad @ 2013-08-09 4:19 UTC (permalink / raw)
To: Darren Etheridge; +Cc: devicetree, tomi.valkeinen, plagnioj, linux-fbdev, afzal
In-Reply-To: <1375992936-16755-2-git-send-email-detheridge@ti.com>
Hi Darren,
Thanks for the patch, below are few nits!
On Fri, Aug 9, 2013 at 1:45 AM, Darren Etheridge <detheridge@ti.com> wrote:
> Enhancing driver to enable probe triggered by a corresponding dt entry.
>
> Add da8xx-fb.txt documentation to devicetree section.
>
> Obtain fb_videomode details for the connected lcd panel using the
> display timing details present in DT.
>
> Ensure that platform data is present before checking whether platform
> callback is present (the one used to control backlight). So far this
> was not an issue as driver was purely non-DT triggered, but now DT
> support has been added this check must be performed.
>
> v2: squashing multiple commits from Afzal Mohammed (afzal@ti.com)
> v3: remove superfluous cast
> v4: expose both ti,am3352-lcdc and ti,da830-lcdc for .compatible
> as driver can use enhanced features of all version of the
> silicon block.
>
> Signed-off-by: Darren Etheridge <detheridge@ti.com>
> ---
> .../devicetree/bindings/video/fb-da8xx.txt | 37 +++++++++++
> drivers/video/da8xx-fb.c | 67 +++++++++++++++++++-
> 2 files changed, 101 insertions(+), 3 deletions(-)
> create mode 100644 Documentation/devicetree/bindings/video/fb-da8xx.txt
>
> diff --git a/Documentation/devicetree/bindings/video/fb-da8xx.txt b/Documentation/devicetree/bindings/video/fb-da8xx.txt
> new file mode 100644
> index 0000000..a6cfe3c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/video/fb-da8xx.txt
> @@ -0,0 +1,37 @@
> +TI LCD Controller on DA830/DA850/AM335x SoC's
> +
> +Required properties:
> +- compatible:
> + DA830 - "ti,da830-lcdc"
> + AM335x SoC's - "ti,am3352-lcdc"
> +- reg: Address range of lcdc register set
> +- interrupts: lcdc interrupt
> +- display-timings: typical videomode of lcd panel, represented as child.
> + Refer Documentation/devicetree/bindings/video/display-timing.txt for
> + display timing binding details. If multiple videomodes are mentioned
> + in display timings node, typical videomode has to be mentioned as the
> + native mode or it has to be first child (driver cares only for native
> + videomode).
> +
> +Example:
> +
> +lcdc@4830e000 {
> + compatible = "ti,am3352-lcdc";
> + reg = <0x4830e000 0x1000>;
> + interrupts = <36>;
> + display-timings {
> + 800x480p62 {
> + clock-frequency = <30000000>;
> + hactive = <800>;
> + vactive = <480>;
> + hfront-porch = <39>;
> + hback-porch = <39>;
> + hsync-len = <47>;
> + vback-porch = <29>;
> + vfront-porch = <13>;
> + vsync-len = <2>;
> + hsync-active = <1>;
> + vsync-active = <1>;
> + };
> + };
> +};
> diff --git a/drivers/video/da8xx-fb.c b/drivers/video/da8xx-fb.c
> index ea9771f..64fd8af 100644
> --- a/drivers/video/da8xx-fb.c
> +++ b/drivers/video/da8xx-fb.c
> @@ -36,6 +36,7 @@
> #include <linux/slab.h>
> #include <linux/delay.h>
> #include <linux/lcm.h>
> +#include <video/of_display_timing.h>
> #include <video/da8xx-fb.h>
> #include <asm/div64.h>
>
> @@ -1297,12 +1298,57 @@ static struct fb_ops da8xx_fb_ops = {
> .fb_blank = cfb_blank,
> };
>
> +static struct lcd_ctrl_config *da8xx_fb_create_cfg(struct platform_device *dev)
> +{
> + struct lcd_ctrl_config *cfg;
> +
> + cfg = devm_kzalloc(&dev->dev, sizeof(struct fb_videomode), GFP_KERNEL);
> + if (!cfg) {
> + dev_err(&dev->dev, "memory allocation failed\n");
devm_* api will print the message if allocation has failed, please remove the
above err statement and also similarly below.
> + return NULL;
> + }
> +
> + /* default values */
> +
> + if (lcd_revision = LCD_VERSION_1)
> + cfg->bpp = 16;
> + else
> + cfg->bpp = 32;
> +
> + /*
> + * For panels so far used with this LCDC, below statement is sufficient.
> + * For new panels, if required, struct lcd_ctrl_cfg fields to be updated
> + * with additional/modified values. Those values would have to be then
> + * obtained from dt(requiring new dt bindings).
> + */
> +
> + cfg->panel_shade = COLOR_ACTIVE;
> +
> + return cfg;
> +}
> +
> static struct fb_videomode *da8xx_fb_get_videomode(struct platform_device *dev)
> {
> struct da8xx_lcdc_platform_data *fb_pdata = dev->dev.platform_data;
> struct fb_videomode *lcdc_info;
> + struct device_node *np = dev->dev.of_node;
> int i;
>
> + if (np) {
> + lcdc_info = devm_kzalloc(&dev->dev,
> + sizeof(struct fb_videomode),
> + GFP_KERNEL);
> + if (!lcdc_info) {
> + dev_err(&dev->dev, "memory allocation failed\n");
ditto
> + return NULL;
> + }
> + if (of_get_fb_videomode(np, lcdc_info, OF_USE_NATIVE_MODE)) {
> + dev_err(&dev->dev, "timings not available in DT\n");
> + return NULL;
> + }
> + return lcdc_info;
> + }
> +
> for (i = 0, lcdc_info = known_lcd_panels;
> i < ARRAY_SIZE(known_lcd_panels); i++, lcdc_info++) {
> if (strcmp(fb_pdata->type, lcdc_info->name) = 0)
> @@ -1331,7 +1377,7 @@ static int fb_probe(struct platform_device *device)
> int ret;
> unsigned long ulcm;
>
> - if (fb_pdata = NULL) {
> + if (fb_pdata = NULL && !device->dev.of_node) {
> dev_err(&device->dev, "Can not get platform data\n");
> return -ENOENT;
> }
> @@ -1371,7 +1417,10 @@ static int fb_probe(struct platform_device *device)
> break;
> }
>
> - lcd_cfg = (struct lcd_ctrl_config *)fb_pdata->controller_data;
> + if (device->dev.of_node)
> + lcd_cfg = da8xx_fb_create_cfg(device);
> + else
> + lcd_cfg = fb_pdata->controller_data;
>
> if (!lcd_cfg) {
> ret = -EINVAL;
> @@ -1390,7 +1439,7 @@ static int fb_probe(struct platform_device *device)
> par->dev = &device->dev;
> par->lcdc_clk = tmp_lcdc_clk;
> par->lcdc_clk_rate = clk_get_rate(par->lcdc_clk);
> - if (fb_pdata->panel_power_ctrl) {
> + if (fb_pdata && fb_pdata->panel_power_ctrl) {
> par->panel_power_ctrl = fb_pdata->panel_power_ctrl;
> par->panel_power_ctrl(1);
What happens to this data in DT case ?
> }
> @@ -1638,6 +1687,17 @@ static int fb_resume(struct platform_device *dev)
> #define fb_resume NULL
> #endif
>
> +static const struct of_device_id da8xx_fb_of_match[] = {
> + /*
> + * this driver supports version 1 and version 2 of the
> + * Texas Instruments lcd controller (lcdc) hardware block
> + */
> + {.compatible = "ti,da830-lcdc", },
> + {.compatible = "ti,am3352-lcdc", },
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, da8xx_fb_of_match);
> +
you can bound this structure and the macro in IS_ENABLED(CONFIG_OF)
Regards,
--Prabhakar Lad
^ permalink raw reply
* [RFC 00/22] OMAPDSS: DT support
From: Tomi Valkeinen @ 2013-08-09 8:38 UTC (permalink / raw)
To: linux-omap, linux-fbdev, devicetree
Cc: Archit Taneja, Laurent Pinchart, Nishanth Menon, Felipe Balbi,
Santosh Shilimkar, Tony Lindgren, Tomi Valkeinen
Hi,
This is an RFC for OMAP Display DT support. The patches work fine, at least
for me, but they are not perfect. I mostly don't have any clear questions
about specific issues, but I would like to get feedback on the selected
approaches in general, and also ideas how to proceed with the series.
This series contains the following:
DT support for the following OMAP's display subsystem devices:
- DSS
- DISPC
- DPI
- HDMI
- VENC
- DSI
- (DBI/RFBI DT is not yet implemented)
DT support for the following external display devices:
- panel-dsi-cm (Generic DSI command mode panel)
- encoder-tfp410 (DPI-to-DVI encoder)
- connector-dvi
- encoder-tpd12s015 (HDMI level-shifter & ESD protection)
- hdmi-connector
- panel-dpi (Generic DPI panel)
- connector-analog-tv (S-Video & Composite)
DT support for the following boards:
- OMAP4 PandaBoard
- OMAP4 SDP
- OMAP3 BeagleBoard
- OMAP3 Overo with Palo43 LCD expansion-board
The patches are not final, and many contain quite brief descriptions.
Binding descriptions are also still missing. The code and bindings in the
patches should be pretty straightforward, though.
The series is based on v3.11-rc2 + a couple of non-DSS fixes. The series
can also be found from:
git://git.kernel.org/pub/scm/linux/kernel/git/tomba/linux.git work/dss-dev-model-dt
Vocabulary
=====
Display Entity - a hardware entity that takes one or more video streams as
input and outputs one or more video streams.
Upstream Entity - A display entity in the "upstream" side of the video
stream, i.e. closer to the original video source.
Downstream Entity - A display entity in the "downstream" side of the video
stream, i.e. closer to the panel or monitor.
Video pipeline - A chain of multiple display entities, starting from the
SoC, going to the display device the user sees.
Display or Panel - A display entity showing the pixels to the user
Encoder - A display entity that takes video as an input and (usually)
outputs it in some other format.
Connector - HDMI/DVI/etc Connector on the board, to which an external
monitor is connected.
About Stable DT Bindings
============
Generally speaking, the DT bindings should be stable. This brings the
following problems:
We already have DT bindings for some OMAP4 and OMAP3 boards, and OMAP4
boards do not even have board files anymore. There are no display bindings
for those OMAP4 boards, but the display support is currently enabled as a
hack, by calling board-file-like code to add the display devices for the
selected boards. So, when we add the display bindings, we should still
support the current DT files which do not have the display bindings. Which
would mean that we'd need to keep the hacky code forever. Considering the
fact that the hacky code does not work quite correct in all cases, I don't
see keeping it as a very good option.
CDF (Common Display Framework) is in the works, and will most likely have
different or more detailed bindings. Moving to CDF would mean we'd somehow
need to still support the old OMAP bindings. In theory the display DT
bindings should stay the same, as they represent the HW, not any SW
constructs, but in practice I find it hard to believe the OMAP display
bindings would be so perfect that there would be no need for changes.
We most likely should somehow represent DSS clock tree in DT. That is not a
simple task, and when we manage to do it, it again means supporting the DT
files without clock tree data.
All in all, I'm a bit scared to push the display bindings, as it's already
clear there are changes coming. Then again, supporting the current hack for
OMAP4 based boards is not nice at all, and has issues, so it would be really
nice to get OMAP4 boards use proper display bindings.
General description of the DT bindings
===================
All the display entities are represented as DT nodes of their own, and have
a matching Linux driver. The display entities are organized by their control
bus; that is, if a display entity is not controlled via any bus, it's a
platform device, and if, say, it's controlled via i2c device, it's an i2c
device.
The exception to the above are DSI and DBI. DSI and DBI are combined control
and video busses, but the use of the busses for control purposes is not
independent of the video stream. Also, the the busses are, in practice,
one-to-one links. And last, DSI and DBI display entities are often also
controllable via, say, i2c. For these reasons there is no real Linux bus for
DSI and DBI and thus the DSI and DBI devices are either platform devices or
i2c devices.
The display entities are connected via "video-source" property. The
video-source points to the upstream display entity where the video data
comes from, and a chain of display entities thus form a full video pipeline.
All video pipelines end with either a panel or a connector.
All the data related to a display entity, and how it is connected on the
given board, is defined in the DT node of the display entity. This means
that the DT node of the upstream entity does not have to be modified when
adding support for new boards.
As an example, consider OMAP3's DPI and two boards using it for panels.
omap3.dtsi contains a node for the DPI, and the board dts files contain
nodes for their panels. The board dts files do not change anything in the
included DPI node. So:
omap3.dtsi:
dpi: encoder@0 {
compatible = "ti,omap3-dpi";
};
omap3-board1.dts:
lcd0: display@0 {
compatible = "samsung,lte430wq-f0c", "panel-dpi";
video-source = <&dpi>;
data-lines = <24>;
};
omap3-board2.dts:
lcd0: display@0 {
compatible = "samsung,lte430wq-f0c", "panel-dpi";
video-source = <&dpi>;
data-lines = <16>;
};
The logic here is that the boards may have multiple panels that are
connected to the same source, even if the panels can only be used one at a
time. Each panel may thus have different properties for the bus, like the
number of data-lines.
Video bus properties
==========
One question I've been pondering for a long time is related to the bus
between two display entities. As an example, DPI (parallel RGB) requires
configuring the number of datalines used. As described above, the properties
of the video bus are represented in the downstream entity.
This approach has one drawback: how to represent features specific to the
upstream entity? Say, if OMAP's DSI has a bus-related foo-feature, which can
be used in some scenarios, the only place where this foo-feature can be
specified is the OMAP DSI's properties. Not in the DSI Panel's properties,
which in the current model contains properties related to the bus.
So Laurent has proposed to use V4L2-like ports, as described in
Documentation/devicetree/bindings/media/video-interfaces.txt. I have not
implemented such feature for OMAP DSS for the following reasons:
- The current supported displays we use work fine with the current method
- If I were to implement such system, it'd most certainly be different than
what CDF will have.
That said, the port based approach does sound good, and it would also remove
the design issue with OMAP DPI and SDI modules as described later. So maybe
I should just go forward with it and hope that CDF will do it in similar
manner.
DSI Module ID
======
On OMAP4 we have two DSI modules. To configure the clock routing and pin
muxing, we need to know the hardware module ID for the DSI device, i.e. is
this Linux platform device DSI1 or DSI2. The same issue exists with other
SoCs with multiple outputs of the same kind.
With non-DT case, we used the platform device's ID directly. With DT, that
doesn't work. I don't currently have a good solution for this, so as a
temporary solution the DSI driver contains a table, from which it can find
the HW module ID by using the device's base address.
I believe, but I am not totally sure, that we can remove the concept of DSI
module ID if we have a good representation of the DSS clock tree and DSI pin
muxing in DT. The clock tree is probably a huge undertaking, but the pin
muxing should be much easier. However, pinmuxing also is some complications,
like the pins requiring a regulator to be enabled.
Display names and aliases
============
With the board-file based model each display was given a name in the board
file. Additionally, each display was given an alias in the style "displayX",
where X is in incrementing number.
The name could be used by the user to see which display device is what, i.e.
on Pandaboard there are displays names "dvi" and "hdmi".
The DT bindings do not have such a name. It would be simple to add a
"nickname" property to each display node, but it just looked rather ugly so
I have left it out.
Additionally, as there's no clear order in which the displays are created,
and thus the number in "displayX" alias could change semi-randomly, I added
the displays to "aliases" node. This keeps the display number the same over
boots, and also gives us some way to define a default display, i.e. which
display to use initially if the user has not specified it.
omapdss virtual device
===========
In addition to the DSS devices matching to DSS hardware modules, we have a
"virtual" omapdss device which does not match to any actual HW module. The
device is there mostly for legacy reasons, but it has also allowed us to
easily pass platform callbacks. The same device is also present in DT
solution. It is created in code, and not present in DT bindings.
Obviously, this omapdss virtual device is on the hack side, and nobody would
mind if it would disappear.
The following data is passed via omapdss device's platform data:
- OMAP DSS version. In theory, the DSS revision registers should tell us
which features the HW supports. In practice, the HW people have not
bothered to change the revision number each time they've made changes. So
we pass a DSS version from the platform code, based on OMAP revision
number.
- omap_dss_set_min_bus_tput() and omap_pm_get_dev_context_loss_count() to
manage PM
- DSI pin muxing functions.
I have some ideas how to deduce the DSS version by poking to certain DSS
registers, but it is not yet tested so I don't know if it will work.
We could do altogether without omap_pm_get_dev_context_loss_count(), so that
should be removable with some work. I am not sure if
omap_dss_set_min_bus_tput() is supported via standard PM calls or not.
However, the use of set_min_bus_tput() is actually a hack, as we're not
really setting min bus tput. What we want to do is prevent OPP50. DSS clocks
have different maximums on OPP100 and OPP50. So if DSS uses a clock over
OPP50 limit, OPP50 cannot be entered. We prevent OPP50 by setting an
arbitrarily high min bus tput.
The DSI pin muxing should also be solvable with DT based solution, but is
not the most trivial task and needs some work.
So, I presume that at some point we can remove the omapdss device, but in
the current solution it exists for the above reasons.
DSS submodules in DT bindings
==============
The OMAP DSS modules are accessed via L4 or L3, and in that sense they
should be on the same level in the DT bindings. However, we do not have them
in the same level, but there is a main "dss" node, under which all the other
DSS modules reside. The main reason for this is that the main DSS device
needs to be enabled for the other modules to work properly, and having this
structure makes runtime PM handle enabling DSS automatically.
If I recall right, somebody (Paul?) mentioned that in the hardware there is
actually some DSS internal bus, and thus the DT structure is good in that
sense also.
We also have nodes (and matching Linux devices) for DPI and SDI. Neither of
them are actuall a separate IP block in the hardware, but are really more
parts of DSS and maybe DISPC. They are still represented in the same way as
the other DSS modules, to have similar architecture for all DSS outputs. But
as they do not have an address-space of their own, the DT nodes use 0 and 1
as "addresses", i.e. "dpi: encoder@0".
That is rather ugly, and maybe could use some cleaning up. A V4L2 port style
approach would probably allow us to add DPI and SDI ports as part of DSS.
Some of the DSS modules are actually a combination of multiple smaller
modules. For example, the DSI module contains DSI protocol, DSI PHY and DSI
PLL modules, each in their own address space. These could perhaps be
presented as separate DT nodes and Linux devices, but I am not sure if that
is a good approach or not.
DT Node Names
======
I have used the following naming conventions with DT nodes:
- Panels are named "display"
- Connectors are named "connector"
- Encoders are named "encoder"
- DSS and DISPC are "dss" and "dispc", as they don't really match any of the
above
When appropriate, the address part of the node is the base address, as in
"dsi1: encoder@58004000". For platform devices, I have used an increasing
number for the address, as in "tpd12s015: encoder@1".
Final words
=====
So as is evident, I have things in my mind that should be improved. Maybe
the most important question for short term future is:
Can we add DSS DT bindings for OMAP4 as unstable bindings? It would give us
some proper testing of the related code, and would also allow us to remove
the related hacks (which don't even work quite right). However, I have no
idea yet when the unstable DSS bindings would turn stable.
If we shouldn't add the bindings as unstable, when should the bindings be
added? Wait until CDF is in the mainline, and use that?
I have not explained every piece of DSS in detail, as that would result in a
book instead of this email, so feel free to ask for more details about any
part.
And last but not least (for me, at least =), I'm on vacation for the next
two weeks, so answers may be delayed.
Tomi
Tomi Valkeinen (22):
ARM: OMAP: remove DSS DT hack
OMAPDSS: remove DT hacks for regulators
ARM: OMAP2+: add omapdss_init_of()
OMAPDSS: if dssdev->name=NULL, use alias
OMAPDSS: get dssdev->alias from DT alias
OMAPFB: clean up default display search
OMAPFB: search for default display with DT alias
OMAPDSS: Add DT support to DSS, DISPC, DPI, HDMI, VENC
OMAPDSS: Add DT support to DSI
ARM: omap3.dtsi: add omapdss information
ARM: omap4.dtsi: add omapdss information
ARM: omap4-panda.dts: add display information
ARM: omap4-sdp.dts: add display information
ARM: omap3-tobi.dts: add lcd (TEST)
ARM: omap3-beagle.dts: add display information
OMAPDSS: panel-dsi-cm: Add DT support
OMAPDSS: encoder-tfp410: Add DT support
OMAPDSS: connector-dvi: Add DT support
OMAPDSS: encoder-tpd12s015: Add DT support
OMAPDSS: hdmi-connector: Add DT support
OMAPDSS: panel-dpi: Add DT support
OMAPDSS: connector-analog-tv: Add DT support
arch/arm/boot/dts/omap3-beagle.dts | 29 ++++++++
arch/arm/boot/dts/omap3-tobi.dts | 33 ++++++++
arch/arm/boot/dts/omap3.dtsi | 43 +++++++++++
arch/arm/boot/dts/omap4-panda-common.dtsi | 48 ++++++++++++
arch/arm/boot/dts/omap4-sdp.dts | 70 +++++++++++++++++
arch/arm/boot/dts/omap4.dtsi | 59 +++++++++++++++
arch/arm/mach-omap2/board-generic.c | 13 +---
arch/arm/mach-omap2/common.h | 2 +
arch/arm/mach-omap2/display.c | 34 +++++++++
arch/arm/mach-omap2/dss-common.c | 23 ------
arch/arm/mach-omap2/dss-common.h | 2 -
.../video/omap2/displays-new/connector-analog-tv.c | 70 +++++++++++++++++
drivers/video/omap2/displays-new/connector-dvi.c | 49 ++++++++++++
drivers/video/omap2/displays-new/connector-hdmi.c | 36 +++++++++
drivers/video/omap2/displays-new/encoder-tfp410.c | 54 ++++++++++++++
.../video/omap2/displays-new/encoder-tpd12s015.c | 62 +++++++++++++++
drivers/video/omap2/displays-new/panel-dpi.c | 75 +++++++++++++++++++
drivers/video/omap2/displays-new/panel-dsi-cm.c | 87 ++++++++++++++++++++++
drivers/video/omap2/dss/dispc.c | 7 ++
drivers/video/omap2/dss/display.c | 23 +++++-
drivers/video/omap2/dss/dpi.c | 8 ++
drivers/video/omap2/dss/dsi.c | 58 +++++++++++++--
drivers/video/omap2/dss/dss.c | 10 +++
drivers/video/omap2/dss/hdmi.c | 11 +--
drivers/video/omap2/dss/venc.c | 7 ++
drivers/video/omap2/omapfb/omapfb-main.c | 67 ++++++++++++-----
26 files changed, 915 insertions(+), 65 deletions(-)
--
1.8.1.2
^ permalink raw reply
* [RFC 01/22] ARM: OMAP: remove DSS DT hack
From: Tomi Valkeinen @ 2013-08-09 8:38 UTC (permalink / raw)
To: linux-omap, linux-fbdev, devicetree
Cc: Archit Taneja, Laurent Pinchart, Nishanth Menon, Felipe Balbi,
Santosh Shilimkar, Tony Lindgren, Tomi Valkeinen
In-Reply-To: <1376037547-10859-1-git-send-email-tomi.valkeinen@ti.com>
As a temporary solution to enable DSS for selected boards when booting
with DT, a hack was added to board-generic.c in
63d5fc0c2f748e20f38a0a0ec1c8494bddf5c288 (OMAP: board-generic: enable
DSS for panda & sdp boards).
We're now adding proper DT support, so the hack can be removed.
Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
arch/arm/mach-omap2/board-generic.c | 11 +----------
arch/arm/mach-omap2/dss-common.c | 23 -----------------------
arch/arm/mach-omap2/dss-common.h | 2 --
3 files changed, 1 insertion(+), 35 deletions(-)
diff --git a/arch/arm/mach-omap2/board-generic.c b/arch/arm/mach-omap2/board-generic.c
index be5d005..d388d33 100644
--- a/arch/arm/mach-omap2/board-generic.c
+++ b/arch/arm/mach-omap2/board-generic.c
@@ -57,17 +57,8 @@ static void __init omap_generic_init(void)
of_platform_populate(NULL, omap_dt_match_table, NULL, NULL);
- /*
- * HACK: call display setup code for selected boards to enable omapdss.
- * This will be removed when omapdss supports DT.
- */
- if (of_machine_is_compatible("ti,omap4-panda")) {
- omap4_panda_display_init_of();
+ if (of_machine_is_compatible("ti,omap4-panda"))
legacy_init_ehci_clk("auxclk3_ck");
-
- }
- else if (of_machine_is_compatible("ti,omap4-sdp"))
- omap_4430sdp_display_init_of();
else if (of_machine_is_compatible("ti,omap5-uevm"))
legacy_init_ehci_clk("auxclk1_ck");
}
diff --git a/arch/arm/mach-omap2/dss-common.c b/arch/arm/mach-omap2/dss-common.c
index 043e570..f3282ac 100644
--- a/arch/arm/mach-omap2/dss-common.c
+++ b/arch/arm/mach-omap2/dss-common.c
@@ -98,12 +98,6 @@ void __init omap4_panda_display_init(void)
omap_mux_init_gpio(HDMI_GPIO_HPD, OMAP_PIN_INPUT_PULLDOWN);
}
-void __init omap4_panda_display_init_of(void)
-{
- omap_display_init(&omap4_panda_dss_data);
-}
-
-
/* OMAP4 Blaze display data */
#define DISPLAY_SEL_GPIO 59 /* LCD2/PicoDLP switch */
@@ -232,20 +226,3 @@ void __init omap_4430sdp_display_init(void)
omap_mux_init_gpio(HDMI_GPIO_CT_CP_HPD, OMAP_PIN_OUTPUT);
omap_mux_init_gpio(HDMI_GPIO_HPD, OMAP_PIN_INPUT_PULLDOWN);
}
-
-void __init omap_4430sdp_display_init_of(void)
-{
- int r;
-
- r = gpio_request_one(DISPLAY_SEL_GPIO, GPIOF_OUT_INIT_HIGH,
- "display_sel");
- if (r)
- pr_err("%s: Could not get display_sel GPIO\n", __func__);
-
- r = gpio_request_one(DLP_POWER_ON_GPIO, GPIOF_OUT_INIT_LOW,
- "DLP POWER ON");
- if (r)
- pr_err("%s: Could not get DLP POWER ON GPIO\n", __func__);
-
- omap_display_init(&sdp4430_dss_data);
-}
diff --git a/arch/arm/mach-omap2/dss-common.h b/arch/arm/mach-omap2/dss-common.h
index 915f6ff..611b341 100644
--- a/arch/arm/mach-omap2/dss-common.h
+++ b/arch/arm/mach-omap2/dss-common.h
@@ -7,8 +7,6 @@
*/
void __init omap4_panda_display_init(void);
-void __init omap4_panda_display_init_of(void);
void __init omap_4430sdp_display_init(void);
-void __init omap_4430sdp_display_init_of(void);
#endif
--
1.8.1.2
^ permalink raw reply related
* [RFC 02/22] OMAPDSS: remove DT hacks for regulators
From: Tomi Valkeinen @ 2013-08-09 8:38 UTC (permalink / raw)
To: linux-omap, linux-fbdev, devicetree
Cc: Archit Taneja, Laurent Pinchart, Nishanth Menon, Felipe Balbi,
Santosh Shilimkar, Tony Lindgren, Tomi Valkeinen
In-Reply-To: <1376037547-10859-1-git-send-email-tomi.valkeinen@ti.com>
For booting Panda and 4430SDP with DT, while DSS did not support DT, we
had to had small hacks in the omapdss driver to get the regulators. With
DT now supported in DSS, we can remove those hacks.
Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
drivers/video/omap2/dss/dsi.c | 5 -----
drivers/video/omap2/dss/hdmi.c | 5 -----
2 files changed, 10 deletions(-)
diff --git a/drivers/video/omap2/dss/dsi.c b/drivers/video/omap2/dss/dsi.c
index 99a043b..0c8bd1f 100644
--- a/drivers/video/omap2/dss/dsi.c
+++ b/drivers/video/omap2/dss/dsi.c
@@ -1132,11 +1132,6 @@ static int dsi_regulator_init(struct platform_device *dsidev)
return 0;
vdds_dsi = devm_regulator_get(&dsi->pdev->dev, "vdds_dsi");
-
- /* DT HACK: try VCXIO to make omapdss work for o4 sdp/panda */
- if (IS_ERR(vdds_dsi))
- vdds_dsi = devm_regulator_get(&dsi->pdev->dev, "VCXIO");
-
if (IS_ERR(vdds_dsi)) {
DSSERR("can't get VDDS_DSI regulator\n");
return PTR_ERR(vdds_dsi);
diff --git a/drivers/video/omap2/dss/hdmi.c b/drivers/video/omap2/dss/hdmi.c
index 44a885b..1b38f1f 100644
--- a/drivers/video/omap2/dss/hdmi.c
+++ b/drivers/video/omap2/dss/hdmi.c
@@ -338,11 +338,6 @@ static int hdmi_init_regulator(void)
return 0;
reg = devm_regulator_get(&hdmi.pdev->dev, "vdda_hdmi_dac");
-
- /* DT HACK: try VDAC to make omapdss work for o4 sdp/panda */
- if (IS_ERR(reg))
- reg = devm_regulator_get(&hdmi.pdev->dev, "VDAC");
-
if (IS_ERR(reg)) {
DSSERR("can't get VDDA_HDMI_DAC regulator\n");
return PTR_ERR(reg);
--
1.8.1.2
^ permalink raw reply related
* [RFC 03/22] ARM: OMAP2+: add omapdss_init_of()
From: Tomi Valkeinen @ 2013-08-09 8:38 UTC (permalink / raw)
To: linux-omap, linux-fbdev, devicetree
Cc: Archit Taneja, Laurent Pinchart, Nishanth Menon, Felipe Balbi,
Santosh Shilimkar, Tony Lindgren, Tomi Valkeinen
In-Reply-To: <1376037547-10859-1-git-send-email-tomi.valkeinen@ti.com>
omapdss driver uses a omapdss platform device to pass platform specific
function pointers and DSS hardware version from the arch code to the
driver. This device is needed also when booting with DT.
This patch adds omapdss_init_of() function, called from board-generic at
init time, which creates the omapdss device.
Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
arch/arm/mach-omap2/board-generic.c | 2 ++
arch/arm/mach-omap2/common.h | 2 ++
arch/arm/mach-omap2/display.c | 34 ++++++++++++++++++++++++++++++++++
3 files changed, 38 insertions(+)
diff --git a/arch/arm/mach-omap2/board-generic.c b/arch/arm/mach-omap2/board-generic.c
index d388d33..aaf1125 100644
--- a/arch/arm/mach-omap2/board-generic.c
+++ b/arch/arm/mach-omap2/board-generic.c
@@ -61,6 +61,8 @@ static void __init omap_generic_init(void)
legacy_init_ehci_clk("auxclk3_ck");
else if (of_machine_is_compatible("ti,omap5-uevm"))
legacy_init_ehci_clk("auxclk1_ck");
+
+ omapdss_init_of();
}
#ifdef CONFIG_SOC_OMAP2420
diff --git a/arch/arm/mach-omap2/common.h b/arch/arm/mach-omap2/common.h
index dfcc182..377900c 100644
--- a/arch/arm/mach-omap2/common.h
+++ b/arch/arm/mach-omap2/common.h
@@ -300,5 +300,7 @@ extern int omap_dss_reset(struct omap_hwmod *);
/* SoC specific clock initializer */
extern int (*omap_clk_init)(void);
+int __init omapdss_init_of(void);
+
#endif /* __ASSEMBLER__ */
#endif /* __ARCH_ARM_MACH_OMAP2PLUS_COMMON_H */
diff --git a/arch/arm/mach-omap2/display.c b/arch/arm/mach-omap2/display.c
index ff37be1..c62aee0 100644
--- a/arch/arm/mach-omap2/display.c
+++ b/arch/arm/mach-omap2/display.c
@@ -23,6 +23,8 @@
#include <linux/clk.h>
#include <linux/err.h>
#include <linux/delay.h>
+#include <linux/of.h>
+#include <linux/of_platform.h>
#include <video/omapdss.h>
#include "omap_hwmod.h"
@@ -564,3 +566,35 @@ int omap_dss_reset(struct omap_hwmod *oh)
return r;
}
+
+int __init omapdss_init_of(void)
+{
+ int r;
+ enum omapdss_version ver;
+
+ static struct omap_dss_board_info board_data = {
+ .dsi_enable_pads = omap_dsi_enable_pads,
+ .dsi_disable_pads = omap_dsi_disable_pads,
+ .get_context_loss_count = omap_pm_get_dev_context_loss_count,
+ .set_min_bus_tput = omap_dss_set_min_bus_tput,
+ };
+
+ ver = omap_display_get_version();
+
+ if (ver = OMAPDSS_VER_UNKNOWN) {
+ pr_err("DSS not supported on this SoC\n");
+ return -ENODEV;
+ }
+
+ board_data.version = ver;
+
+ omap_display_device.dev.platform_data = &board_data;
+
+ r = platform_device_register(&omap_display_device);
+ if (r < 0) {
+ pr_err("Unable to register omapdss device\n");
+ return r;
+ }
+
+ return 0;
+}
--
1.8.1.2
^ permalink raw reply related
* [RFC 04/22] OMAPDSS: if dssdev->name==NULL, use alias
From: Tomi Valkeinen @ 2013-08-09 8:38 UTC (permalink / raw)
To: linux-omap, linux-fbdev, devicetree
Cc: Archit Taneja, Laurent Pinchart, Nishanth Menon, Felipe Balbi,
Santosh Shilimkar, Tony Lindgren, Tomi Valkeinen
In-Reply-To: <1376037547-10859-1-git-send-email-tomi.valkeinen@ti.com>
To avoid the need for a "nickname" property for each display, change
the display registration so that the display's alias (i.e. "display0"
etc) will be used for the dssdev->name if the display driver didn't
provide a name.
This means that when booting with board files, we will have more
descriptive names for displays, like "lcd1", "hdmi". With DT we'll only
have "display0", etc. But as there are no "nicknames" for things like
serials ports either, I hope we will do fine with this approach.
Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
drivers/video/omap2/dss/display.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/video/omap2/dss/display.c b/drivers/video/omap2/dss/display.c
index fafe7c9..64fb6e5 100644
--- a/drivers/video/omap2/dss/display.c
+++ b/drivers/video/omap2/dss/display.c
@@ -137,6 +137,9 @@ int omapdss_register_display(struct omap_dss_device *dssdev)
snprintf(dssdev->alias, sizeof(dssdev->alias),
"display%d", disp_num_counter++);
+ if (dssdev->name = NULL)
+ dssdev->name = dssdev->alias;
+
if (drv && drv->get_resolution = NULL)
drv->get_resolution = omapdss_default_get_resolution;
if (drv && drv->get_recommended_bpp = NULL)
--
1.8.1.2
^ permalink raw reply related
* [RFC 05/22] OMAPDSS: get dssdev->alias from DT alias
From: Tomi Valkeinen @ 2013-08-09 8:38 UTC (permalink / raw)
To: linux-omap, linux-fbdev, devicetree
Cc: Archit Taneja, Laurent Pinchart, Nishanth Menon, Felipe Balbi,
Santosh Shilimkar, Tony Lindgren, Tomi Valkeinen
In-Reply-To: <1376037547-10859-1-git-send-email-tomi.valkeinen@ti.com>
We currently create a "displayX" style alias name for all displays,
using a number that is incremented for each registered display. With the
new DSS device model there is no clear order in which the displays are
registered, and thus the numbering is somewhat random.
This patch improves the behavior for DT case so that if the displays
have been assigned DT aliases, those aliases will be used as DSS
aliases.
This means that "display0" is always the one specified in the DT alias,
and thus display0 can be used as default display in case the user didn't
specify a default display.
Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
drivers/video/omap2/dss/display.c | 20 ++++++++++++++++++--
1 file changed, 18 insertions(+), 2 deletions(-)
diff --git a/drivers/video/omap2/dss/display.c b/drivers/video/omap2/dss/display.c
index 64fb6e5..a8833ca 100644
--- a/drivers/video/omap2/dss/display.c
+++ b/drivers/video/omap2/dss/display.c
@@ -26,6 +26,7 @@
#include <linux/module.h>
#include <linux/jiffies.h>
#include <linux/platform_device.h>
+#include <linux/of.h>
#include <video/omapdss.h>
#include "dss.h"
@@ -133,9 +134,24 @@ static int disp_num_counter;
int omapdss_register_display(struct omap_dss_device *dssdev)
{
struct omap_dss_driver *drv = dssdev->driver;
+ int id;
- snprintf(dssdev->alias, sizeof(dssdev->alias),
- "display%d", disp_num_counter++);
+ /*
+ * Note: this presumes all the displays are either using DT or non-DT,
+ * which normally should be the case. This also presumes that all
+ * displays either have an DT alias, or none has.
+ */
+
+ if (dssdev->dev->of_node) {
+ id = of_alias_get_id(dssdev->dev->of_node, "display");
+
+ if (id < 0)
+ id = disp_num_counter++;
+ } else {
+ id = disp_num_counter++;
+ }
+
+ snprintf(dssdev->alias, sizeof(dssdev->alias), "display%d", id);
if (dssdev->name = NULL)
dssdev->name = dssdev->alias;
--
1.8.1.2
^ permalink raw reply related
* [RFC 06/22] OMAPFB: clean up default display search
From: Tomi Valkeinen @ 2013-08-09 8:38 UTC (permalink / raw)
To: linux-omap, linux-fbdev, devicetree
Cc: Archit Taneja, Laurent Pinchart, Nishanth Menon, Felipe Balbi,
Santosh Shilimkar, Tony Lindgren, Tomi Valkeinen
In-Reply-To: <1376037547-10859-1-git-send-email-tomi.valkeinen@ti.com>
Separate the code for finding the default display into a function for
clarity and to make it easier to extend it in the future.
Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
drivers/video/omap2/omapfb/omapfb-main.c | 46 ++++++++++++++++++++------------
1 file changed, 29 insertions(+), 17 deletions(-)
diff --git a/drivers/video/omap2/omapfb/omapfb-main.c b/drivers/video/omap2/omapfb/omapfb-main.c
index 27d6905..8bfe973 100644
--- a/drivers/video/omap2/omapfb/omapfb-main.c
+++ b/drivers/video/omap2/omapfb/omapfb-main.c
@@ -2407,6 +2407,34 @@ static int omapfb_init_connections(struct omapfb2_device *fbdev,
return 0;
}
+static struct omap_dss_device *
+omapfb_find_default_display(struct omapfb2_device *fbdev)
+{
+ const char *def_name;
+ int i;
+
+ /* search with the display name from the user or the board file */
+
+ def_name = omapdss_get_default_display_name();
+
+ if (def_name) {
+ for (i = 0; i < fbdev->num_displays; ++i) {
+ struct omap_dss_device *dssdev;
+
+ dssdev = fbdev->displays[i].dssdev;
+
+ if (dssdev->name && strcmp(def_name, dssdev->name) = 0)
+ return dssdev;
+ }
+
+ /* def_name given but not found */
+ return NULL;
+ }
+
+ /* return the first display we have in the list */
+ return fbdev->displays[0].dssdev;
+}
+
static int omapfb_probe(struct platform_device *pdev)
{
struct omapfb2_device *fbdev = NULL;
@@ -2484,23 +2512,7 @@ static int omapfb_probe(struct platform_device *pdev)
for (i = 0; i < fbdev->num_managers; i++)
fbdev->managers[i] = omap_dss_get_overlay_manager(i);
- def_display = NULL;
-
- for (i = 0; i < fbdev->num_displays; ++i) {
- struct omap_dss_device *dssdev;
- const char *def_name;
-
- def_name = omapdss_get_default_display_name();
-
- dssdev = fbdev->displays[i].dssdev;
-
- if (def_name = NULL ||
- (dssdev->name && strcmp(def_name, dssdev->name) = 0)) {
- def_display = dssdev;
- break;
- }
- }
-
+ def_display = omapfb_find_default_display(fbdev);
if (def_display = NULL) {
dev_err(fbdev->dev, "failed to find default display\n");
r = -EPROBE_DEFER;
--
1.8.1.2
^ permalink raw reply related
* [RFC 07/22] OMAPFB: search for default display with DT alias
From: Tomi Valkeinen @ 2013-08-09 8:38 UTC (permalink / raw)
To: linux-omap, linux-fbdev, devicetree
Cc: Archit Taneja, Laurent Pinchart, Nishanth Menon, Felipe Balbi,
Santosh Shilimkar, Tony Lindgren, Tomi Valkeinen
In-Reply-To: <1376037547-10859-1-git-send-email-tomi.valkeinen@ti.com>
Improve the search for the default display in two ways:
* compare the given display name to the display's alias
* if no display name is given, look for "display0" DT alias
Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
drivers/video/omap2/omapfb/omapfb-main.c | 23 ++++++++++++++++++++++-
1 file changed, 22 insertions(+), 1 deletion(-)
diff --git a/drivers/video/omap2/omapfb/omapfb-main.c b/drivers/video/omap2/omapfb/omapfb-main.c
index 8bfe973..961c5c2 100644
--- a/drivers/video/omap2/omapfb/omapfb-main.c
+++ b/drivers/video/omap2/omapfb/omapfb-main.c
@@ -2413,7 +2413,10 @@ omapfb_find_default_display(struct omapfb2_device *fbdev)
const char *def_name;
int i;
- /* search with the display name from the user or the board file */
+ /*
+ * Search with the display name from the user or the board file,
+ * comparing to display names and aliases
+ */
def_name = omapdss_get_default_display_name();
@@ -2425,12 +2428,30 @@ omapfb_find_default_display(struct omapfb2_device *fbdev)
if (dssdev->name && strcmp(def_name, dssdev->name) = 0)
return dssdev;
+
+ if (strcmp(def_name, dssdev->alias) = 0)
+ return dssdev;
}
/* def_name given but not found */
return NULL;
}
+ /* then look for DT alias display0 */
+ for (i = 0; i < fbdev->num_displays; ++i) {
+ struct omap_dss_device *dssdev;
+ int id;
+
+ dssdev = fbdev->displays[i].dssdev;
+
+ if (dssdev->dev->of_node = NULL)
+ continue;
+
+ id = of_alias_get_id(dssdev->dev->of_node, "display");
+ if (id = 0)
+ return dssdev;
+ }
+
/* return the first display we have in the list */
return fbdev->displays[0].dssdev;
}
--
1.8.1.2
^ permalink raw reply related
* [RFC 08/22] OMAPDSS: Add DT support to DSS, DISPC, DPI, HDMI, VENC
From: Tomi Valkeinen @ 2013-08-09 8:38 UTC (permalink / raw)
To: linux-omap, linux-fbdev, devicetree
Cc: Archit Taneja, Laurent Pinchart, Nishanth Menon, Felipe Balbi,
Santosh Shilimkar, Tony Lindgren, Tomi Valkeinen
In-Reply-To: <1376037547-10859-1-git-send-email-tomi.valkeinen@ti.com>
Add the code to make DSS, DISPC, DPI, HDMI and VENC drivers work with
device tree on OMAP3 and OMAP4. The only change is adding the
of_match_tables.
Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
drivers/video/omap2/dss/dispc.c | 7 +++++++
drivers/video/omap2/dss/dpi.c | 8 ++++++++
drivers/video/omap2/dss/dss.c | 10 ++++++++++
drivers/video/omap2/dss/hdmi.c | 6 ++++++
drivers/video/omap2/dss/venc.c | 7 +++++++
5 files changed, 38 insertions(+)
diff --git a/drivers/video/omap2/dss/dispc.c b/drivers/video/omap2/dss/dispc.c
index 02a7340..b4dccb8 100644
--- a/drivers/video/omap2/dss/dispc.c
+++ b/drivers/video/omap2/dss/dispc.c
@@ -3743,12 +3743,19 @@ static const struct dev_pm_ops dispc_pm_ops = {
.runtime_resume = dispc_runtime_resume,
};
+static const struct of_device_id dispc_of_match[] = {
+ { .compatible = "ti,omap3-dispc", },
+ { .compatible = "ti,omap4-dispc", },
+ {},
+};
+
static struct platform_driver omap_dispchw_driver = {
.remove = __exit_p(omap_dispchw_remove),
.driver = {
.name = "omapdss_dispc",
.owner = THIS_MODULE,
.pm = &dispc_pm_ops,
+ .of_match_table = dispc_of_match,
},
};
diff --git a/drivers/video/omap2/dss/dpi.c b/drivers/video/omap2/dss/dpi.c
index a6b331e..6b832c8 100644
--- a/drivers/video/omap2/dss/dpi.c
+++ b/drivers/video/omap2/dss/dpi.c
@@ -30,6 +30,7 @@
#include <linux/platform_device.h>
#include <linux/regulator/consumer.h>
#include <linux/string.h>
+#include <linux/of.h>
#include <video/omapdss.h>
@@ -801,12 +802,19 @@ static int __exit omap_dpi_remove(struct platform_device *pdev)
return 0;
}
+static const struct of_device_id dpi_of_match[] = {
+ { .compatible = "ti,omap3-dpi", },
+ { .compatible = "ti,omap4-dpi", },
+ {},
+};
+
static struct platform_driver omap_dpi_driver = {
.probe = omap_dpi_probe,
.remove = __exit_p(omap_dpi_remove),
.driver = {
.name = "omapdss_dpi",
.owner = THIS_MODULE,
+ .of_match_table = dpi_of_match,
},
};
diff --git a/drivers/video/omap2/dss/dss.c b/drivers/video/omap2/dss/dss.c
index bd01608..d0d61b1 100644
--- a/drivers/video/omap2/dss/dss.c
+++ b/drivers/video/omap2/dss/dss.c
@@ -23,6 +23,7 @@
#define DSS_SUBSYS_NAME "DSS"
#include <linux/kernel.h>
+#include <linux/module.h>
#include <linux/io.h>
#include <linux/export.h>
#include <linux/err.h>
@@ -955,12 +956,21 @@ static const struct dev_pm_ops dss_pm_ops = {
.runtime_resume = dss_runtime_resume,
};
+static const struct of_device_id dss_of_match[] = {
+ { .compatible = "ti,omap3-dss", },
+ { .compatible = "ti,omap4-dss", },
+ {},
+};
+
+MODULE_DEVICE_TABLE(of, dss_of_match);
+
static struct platform_driver omap_dsshw_driver = {
.remove = __exit_p(omap_dsshw_remove),
.driver = {
.name = "omapdss_dss",
.owner = THIS_MODULE,
.pm = &dss_pm_ops,
+ .of_match_table = dss_of_match,
},
};
diff --git a/drivers/video/omap2/dss/hdmi.c b/drivers/video/omap2/dss/hdmi.c
index 1b38f1f..1cea0ab 100644
--- a/drivers/video/omap2/dss/hdmi.c
+++ b/drivers/video/omap2/dss/hdmi.c
@@ -1374,6 +1374,11 @@ static const struct dev_pm_ops hdmi_pm_ops = {
.runtime_resume = hdmi_runtime_resume,
};
+static const struct of_device_id hdmi_of_match[] = {
+ { .compatible = "ti,omap4-hdmi", },
+ {},
+};
+
static struct platform_driver omapdss_hdmihw_driver = {
.probe = omapdss_hdmihw_probe,
.remove = __exit_p(omapdss_hdmihw_remove),
@@ -1381,6 +1386,7 @@ static struct platform_driver omapdss_hdmihw_driver = {
.name = "omapdss_hdmi",
.owner = THIS_MODULE,
.pm = &hdmi_pm_ops,
+ .of_match_table = hdmi_of_match,
},
};
diff --git a/drivers/video/omap2/dss/venc.c b/drivers/video/omap2/dss/venc.c
index 496a106..93e13dd 100644
--- a/drivers/video/omap2/dss/venc.c
+++ b/drivers/video/omap2/dss/venc.c
@@ -980,6 +980,12 @@ static const struct dev_pm_ops venc_pm_ops = {
.runtime_resume = venc_runtime_resume,
};
+
+static const struct of_device_id venc_of_match[] = {
+ { .compatible = "ti,omap3-venc", },
+ {},
+};
+
static struct platform_driver omap_venchw_driver = {
.probe = omap_venchw_probe,
.remove = __exit_p(omap_venchw_remove),
@@ -987,6 +993,7 @@ static struct platform_driver omap_venchw_driver = {
.name = "omapdss_venc",
.owner = THIS_MODULE,
.pm = &venc_pm_ops,
+ .of_match_table = venc_of_match,
},
};
--
1.8.1.2
^ permalink raw reply related
* [RFC 09/22] OMAPDSS: Add DT support to DSI
From: Tomi Valkeinen @ 2013-08-09 8:38 UTC (permalink / raw)
To: linux-omap, linux-fbdev, devicetree
Cc: Archit Taneja, Laurent Pinchart, Nishanth Menon, Felipe Balbi,
Santosh Shilimkar, Tony Lindgren, Tomi Valkeinen
In-Reply-To: <1376037547-10859-1-git-send-email-tomi.valkeinen@ti.com>
Add the code to make the DSI driver work with device tree on OMAP3 and
OMAP4.
A minor hack is needed at the moment in the DSI driver: the DSS driver
needs to know the ID number of a DSI device, as clocks are routed in
different ways to the DSI devices. At the moment we don't have any
proper way to manage this, so this patchs adds a simple lookup table
that is used to deduce the ID from the DSI device's base address.
Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
drivers/video/omap2/dss/dsi.c | 53 ++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 52 insertions(+), 1 deletion(-)
diff --git a/drivers/video/omap2/dss/dsi.c b/drivers/video/omap2/dss/dsi.c
index 0c8bd1f..bccf5fc 100644
--- a/drivers/video/omap2/dss/dsi.c
+++ b/drivers/video/omap2/dss/dsi.c
@@ -38,6 +38,7 @@
#include <linux/slab.h>
#include <linux/debugfs.h>
#include <linux/pm_runtime.h>
+#include <linux/of.h>
#include <video/omapdss.h>
#include <video/mipi_display.h>
@@ -371,6 +372,13 @@ struct dsi_packet_sent_handler_data {
struct completion *completion;
};
+struct dsi_module_id_data {
+ u32 address;
+ int id;
+};
+
+static const struct of_device_id dsi_of_match[];
+
#ifdef DEBUG
static bool dsi_perf;
module_param(dsi_perf, bool, 0644);
@@ -5537,7 +5545,6 @@ static int omap_dsihw_probe(struct platform_device *dsidev)
if (!dsi)
return -ENOMEM;
- dsi->module_id = dsidev->id;
dsi->pdev = dsidev;
dev_set_drvdata(&dsidev->dev, dsi);
@@ -5587,6 +5594,31 @@ static int omap_dsihw_probe(struct platform_device *dsidev)
return r;
}
+ if (dsidev->dev.of_node) {
+ const struct of_device_id *match;
+ const struct dsi_module_id_data *d;
+
+ match = of_match_node(dsi_of_match, dsidev->dev.of_node);
+ if (!match) {
+ DSSERR("unsupported DSI module\n");
+ return -ENODEV;
+ }
+
+ d = match->data;
+
+ while (d->address != 0 && d->address != dsi_mem->start)
+ d++;
+
+ if (d->address = 0) {
+ DSSERR("unsupported DSI module\n");
+ return -ENODEV;
+ }
+
+ dsi->module_id = d->id;
+ } else {
+ dsi->module_id = dsidev->id;
+ }
+
/* DSI VCs initialization */
for (i = 0; i < ARRAY_SIZE(dsi->vc); i++) {
dsi->vc[i].source = DSI_VC_SOURCE_L4;
@@ -5641,6 +5673,7 @@ static int omap_dsihw_probe(struct platform_device *dsidev)
else if (dsi->module_id = 1)
dss_debugfs_create_file("dsi2_irqs", dsi2_dump_irqs);
#endif
+
return 0;
err_probe:
@@ -5694,6 +5727,23 @@ static const struct dev_pm_ops dsi_pm_ops = {
.runtime_resume = dsi_runtime_resume,
};
+static const struct dsi_module_id_data dsi_of_data_omap3[] = {
+ { .address = 0x4804fc00, .id = 0, },
+ { },
+};
+
+static const struct dsi_module_id_data dsi_of_data_omap4[] = {
+ { .address = 0x58004000, .id = 0, },
+ { .address = 0x58005000, .id = 1, },
+ { },
+};
+
+static const struct of_device_id dsi_of_match[] = {
+ { .compatible = "ti,omap3-dsi", .data = dsi_of_data_omap3, },
+ { .compatible = "ti,omap4-dsi", .data = dsi_of_data_omap4, },
+ {},
+};
+
static struct platform_driver omap_dsihw_driver = {
.probe = omap_dsihw_probe,
.remove = __exit_p(omap_dsihw_remove),
@@ -5701,6 +5751,7 @@ static struct platform_driver omap_dsihw_driver = {
.name = "omapdss_dsi",
.owner = THIS_MODULE,
.pm = &dsi_pm_ops,
+ .of_match_table = dsi_of_match,
},
};
--
1.8.1.2
^ permalink raw reply related
* [RFC 10/22] ARM: omap3.dtsi: add omapdss information
From: Tomi Valkeinen @ 2013-08-09 8:38 UTC (permalink / raw)
To: linux-omap, linux-fbdev, devicetree
Cc: Archit Taneja, Laurent Pinchart, Nishanth Menon, Felipe Balbi,
Santosh Shilimkar, Tony Lindgren, Tomi Valkeinen
In-Reply-To: <1376037547-10859-1-git-send-email-tomi.valkeinen@ti.com>
Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
arch/arm/boot/dts/omap3.dtsi | 43 +++++++++++++++++++++++++++++++++++++++++++
1 file changed, 43 insertions(+)
diff --git a/arch/arm/boot/dts/omap3.dtsi b/arch/arm/boot/dts/omap3.dtsi
index 7d95cda..3308b3f 100644
--- a/arch/arm/boot/dts/omap3.dtsi
+++ b/arch/arm/boot/dts/omap3.dtsi
@@ -532,5 +532,48 @@
num-eps = <16>;
ram-bits = <12>;
};
+
+ dss@48050000 {
+ compatible = "ti,omap3-dss", "simple-bus";
+ reg = <0x48050000 0x200>;
+ ti,hwmods = "dss_core";
+ #address-cells = <1>;
+ #size-cells = <1>;
+ ranges;
+
+ dispc@48050400 {
+ compatible = "ti,omap3-dispc";
+ reg = <0x48050400 0x400>;
+ interrupts = <25>;
+ ti,hwmods = "dss_dispc";
+ };
+
+ dpi: encoder@0 {
+ compatible = "ti,omap3-dpi";
+ };
+
+ sdi: encoder@1 {
+ compatible = "ti,omap3-sdi";
+ };
+
+ dsi: encoder@4804fc00 {
+ compatible = "ti,omap3-dsi";
+ reg = <0x4804fc00 0x400>;
+ interrupts = <25>;
+ ti,hwmods = "dss_dsi1";
+ };
+
+ rfbi: encoder@48050800 {
+ compatible = "ti,omap3-rfbi";
+ reg = <0x48050800 0x100>;
+ ti,hwmods = "dss_rfbi";
+ };
+
+ venc: encoder@48050c00 {
+ compatible = "ti,omap3-venc";
+ reg = <0x48050c00 0x100>;
+ ti,hwmods = "dss_venc";
+ };
+ };
};
};
--
1.8.1.2
^ permalink raw reply related
* [RFC 11/22] ARM: omap4.dtsi: add omapdss information
From: Tomi Valkeinen @ 2013-08-09 8:38 UTC (permalink / raw)
To: linux-omap, linux-fbdev, devicetree
Cc: Archit Taneja, Laurent Pinchart, Nishanth Menon, Felipe Balbi,
Santosh Shilimkar, Tony Lindgren, Tomi Valkeinen
In-Reply-To: <1376037547-10859-1-git-send-email-tomi.valkeinen@ti.com>
Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
arch/arm/boot/dts/omap4.dtsi | 59 ++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 59 insertions(+)
diff --git a/arch/arm/boot/dts/omap4.dtsi b/arch/arm/boot/dts/omap4.dtsi
index 22d9f2b..ab19f16 100644
--- a/arch/arm/boot/dts/omap4.dtsi
+++ b/arch/arm/boot/dts/omap4.dtsi
@@ -663,5 +663,64 @@
ram-bits = <12>;
ti,has-mailbox;
};
+
+ dss@58000000 {
+ compatible = "ti,omap4-dss", "simple-bus";
+ reg = <0x58000000 0x80>;
+ ti,hwmods = "dss_core";
+ #address-cells = <1>;
+ #size-cells = <1>;
+ ranges;
+
+ dispc@58001000 {
+ compatible = "ti,omap4-dispc";
+ reg = <0x58001000 0x1000>;
+ interrupts = <0 25 0x4>;
+ ti,hwmods = "dss_dispc";
+ };
+
+ dpi: encoder@0 {
+ compatible = "ti,omap4-dpi";
+ };
+
+ rfbi: encoder@58002000 {
+ compatible = "ti,omap4-rfbi";
+ reg = <0x58002000 0x1000>;
+ ti,hwmods = "dss_rfbi";
+ };
+
+ /*
+ * Accessing venc registers cause a crash on omap4, so
+ * this is disabled for now.
+ */
+ /*
+ venc: encoder@58003000 {
+ compatible = "ti,omap4-venc";
+ reg = <0x58003000 0x1000>;
+ ti,hwmods = "dss_venc";
+ };
+ */
+
+ dsi1: encoder@58004000 {
+ compatible = "ti,omap4-dsi";
+ reg = <0x58004000 0x200>;
+ interrupts = <0 53 0x4>;
+ ti,hwmods = "dss_dsi1";
+ };
+
+ dsi2: encoder@58005000 {
+ compatible = "ti,omap4-dsi";
+ reg = <0x58005000 0x200>;
+ interrupts = <0 84 0x4>;
+ ti,hwmods = "dss_dsi2";
+ };
+
+ hdmi: encoder@58006000 {
+ compatible = "ti,omap4-hdmi";
+ reg = <0x58006000 0x1000>;
+ interrupts = <0 101 0x4>;
+ ti,hwmods = "dss_hdmi";
+ };
+ };
};
};
--
1.8.1.2
^ permalink raw reply related
* [RFC 12/22] ARM: omap4-panda.dts: add display information
From: Tomi Valkeinen @ 2013-08-09 8:38 UTC (permalink / raw)
To: linux-omap, linux-fbdev, devicetree
Cc: Archit Taneja, Laurent Pinchart, Nishanth Menon, Felipe Balbi,
Santosh Shilimkar, Tony Lindgren, Tomi Valkeinen
In-Reply-To: <1376037547-10859-1-git-send-email-tomi.valkeinen@ti.com>
Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
arch/arm/boot/dts/omap4-panda-common.dtsi | 48 +++++++++++++++++++++++++++++++
1 file changed, 48 insertions(+)
diff --git a/arch/arm/boot/dts/omap4-panda-common.dtsi b/arch/arm/boot/dts/omap4-panda-common.dtsi
index faa95b5..fa65b19 100644
--- a/arch/arm/boot/dts/omap4-panda-common.dtsi
+++ b/arch/arm/boot/dts/omap4-panda-common.dtsi
@@ -357,3 +357,51 @@
&usbhsehci {
phys = <&hsusb1_phy>;
};
+
+&dsi1 {
+ vdds_dsi-supply = <&vcxio>;
+};
+
+&dsi2 {
+ vdds_dsi-supply = <&vcxio>;
+};
+
+&hdmi {
+ vdda_hdmi_dac-supply = <&vdac>;
+};
+
+/ {
+ aliases {
+ display0 = &dvi0;
+ display1 = &hdmi0;
+ };
+
+ tfp410: encoder@0 {
+ compatible = "ti,tfp410";
+ video-source = <&dpi>;
+ data-lines = <24>;
+ gpios = <&gpio1 0 0>; /* 0, power-down */
+ };
+
+ dvi0: connector@0 {
+ compatible = "ti,dvi_connector";
+ video-source = <&tfp410>;
+ i2c-bus = <&i2c3>;
+ };
+
+ tpd12s015: encoder@1 {
+ compatible = "ti,tpd12s015";
+
+ video-source = <&hdmi>;
+
+ gpios = <&gpio2 28 0>, /* 60, CT CP HPD */
+ <&gpio2 9 0>, /* 41, LS OE */
+ <&gpio2 31 0>; /* 63, HPD */
+ };
+
+ hdmi0: connector@1 {
+ compatible = "ti,hdmi_connector";
+
+ video-source = <&tpd12s015>;
+ };
+};
--
1.8.1.2
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox