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

[Daniel Vetter <daniel@ffwll.ch>]

On Mon, Mar 31, 2014 at 06:03:24PM -0700, Matt Roper wrote:
> On Fri, Mar 28, 2014 at 09:32:06AM +0100, Daniel Vetter wrote:
> > On Thu, Mar 27, 2014 at 05:44:31PM -0700, Matt Roper wrote:
> ...
> > > +	 * 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.
> > 
> > See drm_helper_disable_unused_functions.
> 
> I think I'm still missing something here; can you clarify what the
> problematic case would be?
> 
> I only see a call to __drm_helper_disable_unused_functions() in the CRTC
> helper in cases where mode_changed = 1, which I don't believe it ever
> should be through the setplane() calls.  We should just be updating the
> framebuffer (and possibly panning) without touching any of the
> connectors, so I don't see how unrelated CRTC's would get disabled.  Am
> I overlooking another case here that the basic refcounting in setplane
> doesn't already handle?

Looking at drm_helper_disable_unused_functions we'll spot

	list_for_each_entry(connector, &dev->mode_config.connector_list, head) {
		if (!connector->encoder)
			continue;
		if (connector->status == connector_status_disconnected)
			connector->encoder = NULL;
	}


So as soon as a connector is disconnected and you do _any_ kind of
->set_config call with modesetting helpers that crtc gets killed. Even if
it's completely unrelated. Dave originally changed this with an imo rather
thin justification:

commit a3a0544b2c84e1d7a2022b558ecf66d8c6a8dd93
Author: Dave Airlie <airlied@redhat.com>
Date:   Mon Aug 31 15:16:30 2009 +1000

    drm/kms: add explicit encoder disable function and detach harder.
    
    For shared tv-out and VGA encoders, we really need to know if
    the encoder is just being switched off temporarily in blanking
    or if we are really disabling it hard.
    
    Also we need to try harder to disconnect encoders from unused
    connectors so we can share more efficently.
    
    (shared encoders stuff is coming in radeon tv-out support)
    
    Signed-off-by: Dave Airlie <airlied@redhat.com>

To me this always smelled like papering over broken userspace. I've killed
this in the i915 modeset rewrite and we didn't really have angry users
scaling our walls. But I'm not sure what'll happen if we do this for all
other drivers.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch