On Fri, Mar 28, 2014 at 4:32 AM, Daniel Vetter <daniel@xxxxxxxx> 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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
