[RFCv3 03/14] drm: Add primary plane helpers

[Daniel Vetter]

On Thu, Mar 20, 2014 at 12:01 AM, Matt Roper <matthew.d.roper at intel.com> wrote:
> On Wed, Mar 19, 2014 at 01:24:23PM +0100, Daniel Vetter wrote:
>> On Tue, Mar 18, 2014 at 05:22:48PM -0700, Matt Roper wrote:
>> > When we expose non-overlay planes to userspace, they will become
>> > accessible via standard userspace plane API's.  We should be able to
>> > handle the standard plane operations against primary planes in a generic
>> > way via the page flip handler and modeset handler.
>> >
>> > Drivers that can program primary planes more efficiently, that want to
>> > use their own primary plane structure to track additional information,
>> > or that don't have the limitations assumed by the helpers are free to
>> > provide their own implementation of some or all of these handlers.
>> >
>> > Signed-off-by: Matt Roper <matthew.d.roper at intel.com>
>>
>> One thing I've noticed with planes is that we don't have any per-plane
>> size limits. Which will be annoying for cursors at least, but looking at
>> intel's hw history there are other planes with limits smaller than
>> dev->mode_config.max_widht/height.
>>
>> I think as part of the universal plane work here we should add this to the
>> getplane ioctl (we can extend at the end with full backwards compat) and
>> use the max_width/height as default for primary/overlay planes and
>> cursor_width/height for cursor planes.
>
> I'm not sure I understand how to (cleanly) extend the existing ioctl
> safely.  Userspace (libdrm) allocates a drm_mode_get_plane structure on
> the stack with a specific size.  If we try to extend this structure to
> return more information, and have the kernel write into the new fields,
> aren't we just going to be spilling over into other userspace stack
> variables if we run an old libdrm on the new kernel?  The only
> approaches I see that could make this work would be huge ugly hacks:
>  * Reclaiming the top few bits of plane_id to use as "I sent you the
>    new, larger structure" capability flags (which assumes plane ID's are
>    always small enough to leave those bits 0 on current kernels; this
>    would effectively reduce our plane ID address space).
>  * Figure out a way to encode extra information as bogus pixel formats
>    and shove it into the format list returned to userspace.  Presumably
>    userspace would just ignore/skip the bogus formats.
>
> I suppose we could add a new GetPlane2 ioctl or something that returned
> more info, but I figured it was probably easier to just shove max plane
> size (and a bunch of other plane capabilities / limitations) into some
> new read-only plane properties.  Read-only properties are easy to extend
> if we find other pieces of information we want to return in the future,
> so that seems like the most natural interface to me.

The size userspace expects is passed in through the ioctl number. It
then zero-extends the structure correctly for both forward and
backwards compatibility. Which means that yeah, we can extend the
struct at the end, recompile libdrm and as long as the kernel treats 0
in the new fields sanely.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch