[RFC 0/4] Atomic Display Framework
Rob Clark
robdclark at gmail.com
Thu Aug 29 05:54:06 PDT 2013
On Thu, Aug 29, 2013 at 3:36 AM, Ville Syrjälä
<ville.syrjala at linux.intel.com> wrote:
> On Wed, Aug 28, 2013 at 11:51:59PM -0400, Rob Clark wrote:
>> On Wed, Aug 28, 2013 at 9:51 PM, Greg Hackmann <ghackmann at google.com> wrote:
>> > Hi,
>> >
>> > ADF is an experimental display framework that I designed after experimenting with a KMS-based hardware composer for Android. ADF started as an proof-of-concept implemented from scratch, so right now it's a separate framework rather than a patchstack on top of KMS. If there's community interest, moving forward I'd like to merge its functionality into KMS rather than keep it as a separate thing.
>> >
>> > I'm going to talk about ADF at the Android and Graphics session at Linux Plumbers. The documentation's not done but I wanted to post these patches to people a heads-up about ADF. If there's interest I can write up some more formal documentation ahead of Plumbers.
>> >
>> > I designed ADF to deal with some serious issues I had working with KMS:
>> >
>> > 1. The API is geared toward updating one object at a time. Android's graphics stack needs the entire screen updated atomically to avoid tearing, and on some SoCs to avoid wedging the display hardware. Rob Clark's atomic modeset patchset worked, but copy/update/commit design meant the driver had to keep a lot more internal state.
>> >
>>
>> I'm not entirely sure how to avoid that, because at least some hw we
>> need to have the entire new-state in order to validate if it is
>> possible.
>
> I guess the only reason adf is a bit different is that there can only be
> one custom (driver specific!) blob in the ioctl, so the driver is just
> free to dump that directly into whatever internal structure it uses to
> store the full state. So it just frees you from the per-prop state
> buildup process.
>
> But if the idea would to be totally driver specific anyway, I wouldn't
> even bother with any of this fancy framework stuff. Just specify some
> massive driver specific structure with a custom ioctl and call it a
> day.
>
>> I'm open to suggestions, of course, but the approach I was
>> going for was to at least do most of the boring work for this in
>> drm/kms core (ie. w/ the drm_{plane,crtc,etc}_state stuff)
>
> I'm of two minds about this. On the other hand it would be great to have
> the core take care of the boring stuff as you say, but on the other hand
> the driver most likely will duplicate a bunch of that stuff to some
> internal structures. So there's a slight danger of getting those two out
> of sync somehow.
re: duplicating a bunch of stuff.. I suppose we could make the state
objects opaque (void *), and split everything in core that deals with
'em into helpers. That seems a bit like overkill (ie. would only make
sense if there was some hw that *only* needed the driver specific
derived parameters), but someone speak up if they have hw like this
and I can try to make all the core state tracker stuff just helpers
instead.
as far as getting out of sync, it should only happen if there is a
driver bug (which should be fixed) or if the validate step fails (in
which case the new state is thrown away anyway)
> But I guess relieving the drivers from the boring stuff wins, and we can
> live with the duplication. Or maybe if we get the core stuff right, we
> can avoid most duplication on the driver side.
>
>> > 2. Some SoCs don't map well to KMS's CRTC + planes + encoders + connector model. At the time I was dealing with hardware where the blending engines didn't have their own framebuffer (they could only scan out constant colors or mix the output of other blocks), and you needed to gang several mixers together to drive high-res displays.
>> >
>>
>> currently you can connect a crtc to multiple encoders (at least for
>> some hw).. I suppose w/ the small addition of having a way to specify
>> an encoder's x/y offset, we could cover this sort of case?
>
> Hmm. Yeah simply killing the fb->crtc link (which is definitely on my
> TODO list for the atomic API) isn't enough to cover this case. At first
> glance the x/y offset in the encoder seems like a reasonable approach
> to me.
>
> But there is one other issue here, and that is how to represent the
> connector level. Do we have multiple active connectors for such
> displays, or just one? If we would go with one connector, we'd need to
> link multiple encoders to the same connector, which isn't possibly
> currently.
I guess if you have multiple encoders + multiple connectors for the
"ganging" case, then it probably just looks like 2x displays, and
nothing more really needed?
I'm a bit fuzzy on what the hw looks like in this "ganging" scenario,
so I'm not completely sure what the best approach is. But if we do
have hw like this, then it makes sense to support it *somehow* in KMS.
> The other option would be to abstract the hardware a bit, and just
> expose one crtc to userspace, but internally build it up from multiple
> mixers.
>
>> > 3. KMS doesn't support custom pixel formats, which a lot of newer SoCs use internally to cut down on bandwidth between hardware blocks.
>>
>> for custom formats, use a custom fourcc value, this should work.
>>
>> err, hmm, looks like some of the error checking that was added is a
>> bit too aggressive. What I'd propose is:
>>
>> #define DRM_FORMAT_CUSTOM 0x80000000
>>
>> and then 'if (format & DRM_FORMAT_CUSTOM) ... pass through to driver ..'
>
> Bit 31 is already taken :) It has to be one of the other three high bits.
oh, right, missed that.. ok, then 0x00800000 ;-)
> The aggresive checking was added precisely to avoid people adding
> formats at a moments whim. We kind of blew that with the exynos NV12MT
> format, but then again that thing still hasn't been added to
> format_check() so I guess it's not _that_ important. Maybe we could
> "rewrite" history a bit and move that to live under the new
> DRM_FORMAT_CUSTOM bit as well (assuming we add such a thing).
>
perhaps even dev->driver->validate_custom_format() could be useful?
we probably do want to keep an eye on custom formats that are added to
make sure they are really actually custom.. but either way, we do want
to allow for custom tiled and/or compressed formats.
>>
>> > 4. KMS doesn't have a way to exchange sync fences. As a hack I managed to pass sync fences into the kernel as properties of the atomic pageflip, but there was no good way to get them back out of the kernel without a side channel.
>> >
>>
>> I was going to recommend property to pass in. Or, we could possibly
>> even just add a generic fence parameter to the pageflip/modeset ioctl,
>> and just pass it through opaquely to the driver. I guess it should
>> not be any harm to the upstream gpu drivers, they'd just ignore it and
>> do same 'ol implicit synchronization.
>>
>> for returning to userspace, just put the new fence value (if there is
>> just one?) in the ioctl as an out param.
>
> Or just add an array of them if we need many. My design for the atomic ioctl
> is already variable size, so adding one more special purpose list of things
> isn't that far fetched, assuming there's a good reason for it. Of course
> the caller has to specify the max number of things it's expecting back
> since it has to reserve enough space for them.
I wasn't completely sure if it was one fence, or N fences. Either
way, drm core doesn't really need to know what they are, just that it
should pass 'em through to the driver. So if it helps align android
and upstream, I'm ok with adding something.
> Another "multiple return values" issue I was thinking is the error
> reporting. It might be semi-useful to hand back per object errors. Then the
> user might be able to just drop those objects from the list and try again.
> But there are probably too many ways that things can still fail, and
> it's just not worth trying to iterate the config too much as that would
> take time, possibly leading to missed deadline.
>
yeah.. maybe not worth it on the page flip due to the time
constraint. At least for mode set, it would be worthwhile to try to
explain to userspace why it failed. (Ie. think 3x display on ivb sort
of scenario where you have inter-dependencies between modes.) I'm ok
to punt on this for v1 though. The nice thing about how drm_ioctl()
works, if some care is taken we can actually extend ioctls rather than
having to add a new ioctl.
BR,
-R
>>
>> BR,
>> -R
>>
>> > ADF represents display devices as collections of overlay engines and interfaces. Overlay engines (struct adf_overlay_engine) scan out images and interfaces (struct adf_interface) display those images. Overlay engines and interfaces can be connected in any n-to-n configuration that the hardware supports.
>> >
>> > Clients issue atomic updates to the screen by passing in a list of buffers (struct adf_buffer) consisting of dma-buf handles, sync fences, and basic metadata like format and size. If this involves composing multiple buffers, clients include a block of custom data describing the actual composition (scaling, z-order, blending, etc.) in a driver-specific format.
>> >
>> > Drivers provide hooks to validate these custom data blocks and commit the new configuration to hardware. ADF handles importing the dma-bufs and fences, waiting on incoming sync fences before committing, advancing the display's sync timeline, and releasing dma-bufs once they're removed from the screen.
>> >
>> > ADF represents pixel formats using DRM-style fourccs, and automatically sanity-checks buffer sizes when using one of the formats listed in drm_fourcc.h. Drivers can support custom fourccs if they provide hooks to validate buffers that use them.
>> >
>> > ADF also provides driver hooks for modesetting, managing and reporting hardware events like vsync, and changing DPMS state. These are documented in struct adf_{obj,overlay_engine,interface,device}_ops, and are similar to the equivalent DRM ops.
>> >
>> > Greg Hackmann (3):
>> > video: add atomic display framework
>> > video: adf: add display core helper
>> > video: adf: add memblock helper
>> >
>> > Laurent Pinchart (1):
>> > video: Add generic display entity core
>> >
>> > drivers/video/Kconfig | 2 +
>> > drivers/video/Makefile | 2 +
>> > drivers/video/adf/Kconfig | 15 +
>> > drivers/video/adf/Makefile | 14 +
>> > drivers/video/adf/adf.c | 1013 ++++++++++++++++++++++++++++++++++
>> > drivers/video/adf/adf.h | 49 ++
>> > drivers/video/adf/adf_client.c | 853 ++++++++++++++++++++++++++++
>> > drivers/video/adf/adf_display.c | 123 +++++
>> > drivers/video/adf/adf_fops.c | 982 ++++++++++++++++++++++++++++++++
>> > drivers/video/adf/adf_fops.h | 37 ++
>> > drivers/video/adf/adf_fops32.c | 217 ++++++++
>> > drivers/video/adf/adf_fops32.h | 78 +++
>> > drivers/video/adf/adf_memblock.c | 150 +++++
>> > drivers/video/adf/adf_sysfs.c | 291 ++++++++++
>> > drivers/video/adf/adf_sysfs.h | 33 ++
>> > drivers/video/adf/adf_trace.h | 93 ++++
>> > drivers/video/display/Kconfig | 4 +
>> > drivers/video/display/Makefile | 1 +
>> > drivers/video/display/display-core.c | 362 ++++++++++++
>> > include/video/adf.h | 743 +++++++++++++++++++++++++
>> > include/video/adf_client.h | 61 ++
>> > include/video/adf_display.h | 31 ++
>> > include/video/adf_format.h | 282 ++++++++++
>> > include/video/adf_memblock.h | 20 +
>> > include/video/display.h | 150 +++++
>> > 25 files changed, 5606 insertions(+)
>> > create mode 100644 drivers/video/adf/Kconfig
>> > create mode 100644 drivers/video/adf/Makefile
>> > create mode 100644 drivers/video/adf/adf.c
>> > create mode 100644 drivers/video/adf/adf.h
>> > create mode 100644 drivers/video/adf/adf_client.c
>> > create mode 100644 drivers/video/adf/adf_display.c
>> > create mode 100644 drivers/video/adf/adf_fops.c
>> > create mode 100644 drivers/video/adf/adf_fops.h
>> > create mode 100644 drivers/video/adf/adf_fops32.c
>> > create mode 100644 drivers/video/adf/adf_fops32.h
>> > create mode 100644 drivers/video/adf/adf_memblock.c
>> > create mode 100644 drivers/video/adf/adf_sysfs.c
>> > create mode 100644 drivers/video/adf/adf_sysfs.h
>> > create mode 100644 drivers/video/adf/adf_trace.h
>> > create mode 100644 drivers/video/display/Kconfig
>> > create mode 100644 drivers/video/display/Makefile
>> > create mode 100644 drivers/video/display/display-core.c
>> > create mode 100644 include/video/adf.h
>> > create mode 100644 include/video/adf_client.h
>> > create mode 100644 include/video/adf_display.h
>> > create mode 100644 include/video/adf_format.h
>> > create mode 100644 include/video/adf_memblock.h
>> > create mode 100644 include/video/display.h
>> >
>> > --
>> > 1.8.0
>> >
>> > _______________________________________________
>> > dri-devel mailing list
>> > dri-devel at lists.freedesktop.org
>> > http://lists.freedesktop.org/mailman/listinfo/dri-devel
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>
> --
> Ville Syrjälä
> Intel OTC
More information about the dri-devel
mailing list