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

[Rob Clark]

On Tue, Apr 1, 2014 at 3:45 AM, 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:

sure, so this could change the timing of things a bit..  but it would
be a bug if a HPD event or poll didn't eventually detect that and shut
things off..

BR,
-R

> 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.
> -Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel