On Fri, Mar 28, 2014 at 4:32 AM, Daniel Vetter daniel@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