[RFC] Updated plane support v3

[Rob Clark]

On Tue, Jun 21, 2011 at 6:21 AM, Rob Clark <robdclark at gmail.com> wrote:
> On Mon, Jun 20, 2011 at 3:11 PM, Jesse Barnes <jbarnes at virtuousgeek.org> wrote:
>> This version adds both source and dest rect params to the set_plane
>> ioctl, and makes the source fixed point to support hardware that needs
>> it.
>>
>> I haven't changed the name of the SNB implementation yet (per Chris's
>> suggestions) but will before it gets upstream.
>>
>> I'd be interested to see whether these interfaces will work for other
>> hardware, so please take a close look at them and ideally implement them
>> on your hardware to make sure (see my userspace example code from
>> earlier posts if you want something to crib from).
>
> Cool, thanks for this
>
> I'm just thinking through how I'd implement the driver part in omap
> drm driver.. so please bear with me if I'm misunderstanding..
>
> In particular I'm thinking about being able to use a given video pipe
> (basically like a dma channel) as either framebuffer layer or overlay
> at various points in time, depending on how many displays are
> attached.  Is the idea to use drm_plane *only* for overlay layer, and
> still use crtc->fb for the normal framebuffer layer?
>
> Or would/could a drm_plane also be used for main layer instead of
> crtc->fb?  In this case, either userspace would have to know (which
> doesn't seem like a good idea for things like plymouth which use drm
> interface a bit generically).  Or the driver would have to internally
> automatically hook up a drm_plane if it sees that userspace hasn't
> done this.
>
> I was thinking perhaps that if we let userspace DRM_IOCTL_MODE_SETCRTC
> pass in -1 for fb_id, followed by one or more
> DRM_IOCTL_MODE_SETPLANE's, to set things up the "new" way (explicitly
> specify the drm_plane's).  Or if _SETCRTC passes in a valid fb_id, we
> know it is the old way, and driver automatically picks a drm_plane.

just thinking through this a bit more..  the idea is that for drivers
w/ DRM_SUPPORTS_PLANES[1], there is no fb hooked directly to crtc.
And support for userspace that is not aware of planes could be, I
think, mostly in the kms core, although I'm just thinking through how
disruptive that would be..

I think if we add a crtc->funcs->get_primary_plane() to be implemented
by drivers that use planes in cases where userspace does not know
about planes.  It would be used to get (and if necessary, assign) a
drm_plane to a drm_crtc, then we can do things like:

handle _SETCRTC from userspace that doesn't understand planes:
---------
@@ -1580,7 +1580,14 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data,
        set.mode = mode;
        set.connectors = connector_set;
        set.num_connectors = crtc_req->count_connectors;
-       set.fb = fb;
+       if (fb & (dev->driver->driver_features & DRM_SUPPORTS_PLANES)) {
+               /* pass fb to the primary plane for compatibility */
+               struct drm_plane *plane =
+                               crtc->funcs->get_primary_plane(crtc);
+               set.plane = plane;
+               plane->funcs->set_config(&set);
+       } else {
+               set.fb = fb;
+       }
        ret = crtc->funcs->set_config(&set);

 out:
---------

or page flip ioctl from userspace that doesn't understand planes:

---------
@@ -2645,7 +2652,14 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
                        (void (*) (struct drm_pending_event *)) kfree;
        }

-       ret = crtc->funcs->page_flip(crtc, fb, e);
+       if (dev->driver->driver_features & DRM_SUPPORTS_PLANES) {
+               struct drm_plane *plane =
+                               crtc->funcs->get_primary_plane(crtc);
+               ret = plane->funcs->page_flip(plane, fb, e);
+       } else {
+               ret = crtc->funcs->page_flip(crtc, fb, e);
+       }
+
        if (ret) {
                spin_lock_irqsave(&dev->event_lock, flags);
                file_priv->event_space += sizeof e->event;
---------

It doesn't look like there are *that* many places where crtc->fb is
reference, but I've not gone through all of them..  I'm not sure if
this sort of change would be too intrusive.

BR,
-R

--
[1] or maybe DRM_MANDATORY_PLANES if there are some drivers that would
use a hybrid model of fb either direct to drm_crtc, or indirect via a
drm_plane

BR,
-R