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

[Dave Airlie]

On Tue, Apr 1, 2014 at 5:45 PM, Daniel Vetter <daniel at ffwll.ch> wrote:
> 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 at 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 at 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.

I've had to look at this again recently, and while I still don't like
my commit, its
not papering over userspace, it might be papering over fbcon :-)

You don't have any hw that operates like this, so I'd be surprised if
you had users falling over,

The problem is if I have a single DAC encoder, with tv-out and VGA
connectors, and I unplug the VGA connector, and plug in the tv
connector, how do I get fbcon to pop-up,

Though that said this commit caused a regression that I'm not sure I
liked either, since I think we used to allow you to force a mode on
disconnected outputs, and this stops that from working, I noticed in a
virtual env.

Dave.