[RFC] drm: Introduce max width and height properties for planes

[Daniel Vetter]

On Mon, May 30, 2016 at 02:09:17PM +0530, Archit Taneja wrote:
> 
> 
> On 05/25/2016 10:06 PM, Rob Clark wrote:
> >On Wed, May 25, 2016 at 9:12 AM, Daniel Vetter <daniel at ffwll.ch> wrote:
> >>On Wed, May 25, 2016 at 04:28:36PM +0530, Archit Taneja wrote:
> >>>On 05/25/2016 12:40 PM, Daniel Vetter wrote:
> >>>>- Is the size/width really independent of e.g. rotation/pixel format/...
> >>>>   Should it be the maximum that's possible under best circumstance (things
> >>>>   could still fail), or the minimum that's guaranteed to work everwhere.
> >>>>   This is the kind of stuff we need the userspace part for, too.
> >>>
> >>>Yeah, it isn't independent of these parameters. I'm not entirely sure
> >>>about this either.
> >>>
> >>>Does it make sense to impose a rule that the user first sets the
> >>>rotation/format plane properties, and only then read the maximum
> >>>width? I'm assuming user space hates such kind of stuff.
> >>>
> >>>If we use the 'best circumstance' max_width, we can first start
> >>>with a minimum number of planes that need to be grouped to achieve
> >>>the target mode. If that fails the atomic test, then we can try to
> >>>add one plane at a time, and reduce the width for each plane.
> >>>
> >>>If we use the minimum/'guaranteed to work' max_width, we'll get
> >>>a higher number of planes than needed for this mode. This would pass
> >>>the atomic test. We could then reduce a plane at a time and see when
> >>>we fail the atomic test.
> >>>
> >>>I guess we need to chose the one that's more probable to get right
> >>>the first time. Considering only pixel formats for now, the
> >>>minimum/'guaranteed to work' would map to the RGB formats. The best
> >>>circumstance ones would probably be the planar video formats. Since
> >>>we use RGB more often, the minimum one might make more sense.
> >>>
> >>>We could, of course, give the property a range of max widths to
> >>>confuse user space even more.
> >>
> >>An entirely different idea for cases where a simple hint property doesn't
> >>work (other ideas floating around are can_scale, to give a hint whether a
> >>plan can at least in theory up/downscale, or not at all), is that the
> >>kernel gives more specific hints about what it would like to change.
> >>
> >>So if userspace asks for a plane, but for the given pixel format it's too
> >>wide, the kernel could return a new proposed value for width. That would
> >>be super-flexible and could cover all kinds of use-case like rotation
> >>needing a specific tiling (fb_modifier) or specific pixel format, or
> >>specific stride.
> 
> This would be great to have, but it sounds like a new class of ABI
> altogether, where you set a configuration, the kernel rejects it, but
> gives a hint about what it wants. Do we already have something in DRI
> that follows such a mechanism?

Yeah, we definitely don't want to start out with that.

> >>For the case at hand there's even more worms: What about stride
> >>requirements? Afaik on some hw you just need to split the buffers into 2
> >>planes, but can keep the wide stride (since the limit is the size of the
> >>linebuffers in the hw). On others you need to split the buffer itself into
> >>2, because the plane hw can't cope with huge strides. Again might depend
> >>upon the pixel format.
> 
> I assumed that hw always belonged to the first category. For the
> latter, the userspace strategy of allocating buffers would have to
> change itself.
> 
> If we do decide to hide pipe virtualization in the kernel as you
> mentioned below, how would we manage plane hw that can't do huge
> strides. Would we need to copy the userspace populated buffer into
> two separate kernel buffers that fit the stride requirements?

I think at that point the kernel needs to make sure that the preferred
mode is _not_ a mode that requires such a stride. And then userspace needs
to piece stuff together on it's own. This is hw-mis-design on a level that
I don't think makes much sense to abstract away ;-)

But afaik the discussion here is just for hw where we might need 2 or 4
planes to scan out one logical buffer.

> >>So in a way height/width is both too much information and not precise
> >>enough. Entirely different approches:
> >>- We just add a might_need_split_plane prop to crtcs where this might be
> >>   needed. Userspace then gets to retry with split buffers if it doesn't
> >>   work with a huge one.
> >>
> >>- When I discussed this with qualcom folks for msm we concluded that the
> >>   simplest approach would be to hide this in the kernel. So if you have a
> >>   too wide plane, and need 2 hw planes to scan it out, then do that
> >>   transparently in the kernel. Of course this means that there will be 1
> >>   (or 3 if you need a 2x2 split) fewer planes available, but userspace
> >>   needs to iteratively build up the plane config using ATOMIC_TEST anyway.
> >
> >Just fwiw, there are a few things that we will still end up
> >abstracting in the kernel by virtualizing the mapping between kms
> >planes and hw pipes.  And the approach of weston atomic of
> >incrementally adding more planes w/ TESTONLY flag should work well for
> >that.  (Let's hope the weston bits get upstream some day..)
> >
> >But exposing width limit avoids the one-plane to multiple-pipes case,
> >considerably simplifying things.  And seemed like a generic enough
> >limit (iirc, it applies to omapdss and probably others), that it would
> >be cleaner to expose the limit to userspace.  So there should be at
> >least a couple other drivers that could avoid virtualizing planes with
> >some help from userspace for this case.
> >
> >Regarding rotation, I'm not 100% sure.. seems like we could just
> >document these as the un-rotated limits.  If we really had to, we
> >could do some sort of dance where userspace sets rotation property on
> >an un-used plane, and then reads-back the current values of the
> >read-only prop's.  But that seems awkward.
> >
> >BR,
> >-R
> >
> >>If possible for your hw I'm heavily leaning towards this last approach. If
> >>fits entirely into the current atomic design, and all the complexity is
> >>restricted to your driver (you need to have some allocation map between
> >>drm planes and real hw planes, but that's it).
> 
> I guess the last approach seems more apt for compositors that run the
> same code for all hw (weston, etc). In android userspace, each display
> hw has the luxury of having it's own middleware where they have more freedom
> on how they manage planes and how they interpret the DRM
> ioctls. For such middleware, preventing virtualization helps in moving
> the bulk of the code in userspace. I don't know if android will
> continue to have such middleware when all SoCs move to drm hwcomposer,
> though.

drm_hwcomposer (developed by Google) is a generic hwc implementation along
the lines of weston and any other generic kms compositor. I think it makes
a lot of sense to aim for a reasonable baseline API which works the same
way everywhere. And that reasonable baseline abi imo includes that at
least 1 plane can be used full-screen, without jumping through hooks.

Because if you don't have that, then it's not just weston and android you
need to patch up. But also
- fbdev
- any boot splash
- any igt testcases (I'm starting to push hard to make those a requirement
  for kms drivers)
- X
- any other compositor, like mutter, kwin, ozone, ...

At that point we're not doing a real generic platform any more, but fully
leaking all the details into userspace (like hwc does on Android). That's
not terribly useful.

Imo the rule should be that special code can only be required by userspace
for optimizations (more power efficient or whatever), not for basic
functionality. Displaying a boot splash is _very_ basic ;-)

> This probably needs more feedback Qcom and other SoC people. Although,
> as Rob said, if a lot of hw has a limitation over the line buffer width
> they have on a plane, it could be handy to have such expose such a
> property for drivers that don't want to virtualize their plane hw just
> for this one usecase. But, as you said, max_width isn't precise enough
> information either. So, yeah, we'll probably need to think over this
> a bit more.

I massively disagree with "this one usecase". If you can't do all the
things above, there's no reason at all to have a kms driver imo. At least
not in upstream. If it only works on that one vendor tree with the one
vendor hwc and no where else, I don't think it makes sense to have it in
upstream.

But like discussed with Rob, I think it should be pretty easy to have a
set of generic helpers in-kernel to make this virtualization really easy.
In the end you want to share the code between different compositors
anyway, and the best place to put such shared code is within the kernel
itself imo. Of course for such a helper we'd need to have 2-3 drivers
using it, to make sure it's good enough.

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch