[PATCHv4 06/13] drm: Add primary plane helpers (v2)

[Rob Clark]

On Fri, Mar 28, 2014 at 4:32 AM, Daniel Vetter <daniel at ffwll.ch> wrote:
>
>> +     /*
>> +      * set_config() adjusts crtc->primary->fb; however the DRM setplane
>> +      * code that called us expects to handle the framebuffer update and
>> +      * reference counting; save and restore the current fb before
>> +      * calling it.
>> +      *
>> +      * N.B., we call set_config() directly here rather than using
>> +      * drm_mode_set_config_internal.  We're reprogramming the same
>> +      * connectors that were already in use, so we shouldn't need the extra
>> +      * cross-CRTC fb refcounting to accomodate stealing connectors.
>> +      * drm_mode_setplane() already handles the basic refcounting for the
>> +      * framebuffers involved in this operation.
>
> Wrong. The current crtc helper logic disables all disconnected connectors
> forcefully on each set_config. Nope, I didn't make those semantics ... So
> I think we need to think a bit harder here again.

Hmm, I'm not really seeing the problem.  The worst setplane could do
is enable a crtc which otherwise already has an encoder+connector
pointing to it.  Which would mean that encoder+connector could not be
pointing to another crtc.

So yeah, we'll end up calling dpms(OFF) on a bunch of stuff that is
already off.. is that a problem?

>
> See drm_helper_disable_unused_functions.
>
>> +      */
>> +     tmpfb = plane->fb;
>> +     ret = crtc->funcs->set_config(&set);
>> +     plane->fb = tmpfb;
>> +
>> +     kfree(connector_list);
>> +     return ret;
>> +}
>> +EXPORT_SYMBOL(drm_primary_helper_update);
>> +
>> +/**
>> + * drm_primary_helper_disable() - Helper for primary plane disable
>> + * @plane: plane to disable
>> + *
>> + * Provides a default plane disable handler for primary planes.  This is handler
>> + * is called in response to a userspace SetPlane operation on the plane with a
>> + * NULL framebuffer parameter.  We call the driver's modeset handler with a NULL
>> + * framebuffer to disable the CRTC if no other planes are currently enabled.
>> + * If other planes are still enabled on the same CRTC, we return -EBUSY.
>> + *
>> + * Note that some hardware may be able to disable the primary plane without
>> + * disabling the whole CRTC.  Drivers for such hardware should provide their
>> + * own disable handler that disables just the primary plane (and they'll likely
>> + * need to provide their own update handler as well to properly re-enable a
>> + * disabled primary plane).
>> + *
>> + * RETURNS:
>> + * Zero on success, error code on failure
>> + */
>> +int drm_primary_helper_disable(struct drm_plane *plane)
>> +{
>> +     struct drm_plane *p;
>> +     struct drm_mode_set set = {
>> +             .crtc = plane->crtc,
>> +             .fb = NULL,
>> +     };
>> +
>> +     if (plane->crtc == NULL || plane->fb == NULL)
>> +             /* Already disabled */
>> +             return 0;
>> +
>> +     list_for_each_entry(p, &plane->dev->mode_config.plane_list, head)
>> +             if (p != plane && p->fb) {
>> +                     DRM_DEBUG_KMS("Cannot disable primary plane while other planes are still active on CRTC.\n");
>> +                     return -EBUSY;
>> +             }
>> +
>> +     /*
>> +      * N.B.  We call set_config() directly here rather than
>> +      * drm_mode_set_config_internal() since drm_mode_setplane() already
>> +      * handles the basic refcounting and we don't need the special
>> +      * cross-CRTC refcounting (no chance of stealing connectors from
>> +      * other CRTC's with this update).
>
> Same comment as above applies. Calling ->set_config is considered harmful
> ... Maybe we need to add another wrapper here for the primary plane
> helpers to wrap this all up safely?


actually, again, I think calling .set_config() directly here is the
correct thing.  There should be no connector/encoder changes.  and the
drm_mode_set_config_internal() refcounting would be wrong for
drm_mode_setplane().  In the helpers he undoes the plane->fb update
done by the driver in .set_config() so that drm_mode_setplane() dtrt..


As an aside, that inconsistency between who updates the fb ptr between
setcrtc and setplane has been bothering me for a while now with the
atomic stuff.  Maybe it's just the OCD talking, but I'd *really* like
to clean that up.  The rough thinking I have, with the atomic stuff we
introduce a second fb pointer (and reference) in the state object.  So
initially we have:

   plane->fb    ---   current rules
   plane->state->fb   ---   immutable, set to new incoming fb before
calling in to driver.. drm core manages the refcnt, read-only access
for the driver

what I'd like to do is request that all drivers add their own internal
scanout references, etc.  And we eventually remove plane->fb when no
one still references it.  The outgoing state object will hold a ref to
old_fb until after we return from calling in to the driver.  The only
thing left is for each driver to (if it cannot cancel scanout
mid-frame) hold a reference until next vblank.

BR,
-R