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

[Daniel Vetter]

On Tue, Apr 1, 2014 at 1:45 PM, Rob Clark <robdclark at gmail.com> wrote:
> 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..

Well on the semantics imo this is userspace's decision, or should be
at least. And there's no driver which kills the output in the hdp
handler at all. So I'm all in favour of just removing this and letting
DEs handle hpd events (like they currently all do). But until we have
that completely unrelated crtcs _can_ get disabled when you call
->set_config, and that means we do have to handle the refcounting
correctly by doing what set_config_internal does essentially.
-Daniel
--
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch