[PATCH 13/15] drm: take references to connectors used in a modeset.

[Dave Airlie]

On 22 April 2016 at 18:49, Daniel Vetter <daniel at ffwll.ch> wrote:
> On Fri, Apr 15, 2016 at 03:10:44PM +1000, Dave Airlie wrote:
>> From: Dave Airlie <airlied at redhat.com>
>>
>> As suggested by Daniel, if we are actively using the connector in a modeset
>> we don't want it to disappear from underneath us. This takes a reference
>> to the connector in the atomic paths when we are setting the state up,
>> and in the non-atomic paths when binding the encoder.
>>
>> Signed-off-by: Dave Airlie <airlied at redhat.com>
>> ---
>>  drivers/gpu/drm/drm_atomic.c      | 11 ++++++++++-
>>  drivers/gpu/drm/drm_crtc_helper.c |  6 ++++++
>>  2 files changed, 16 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
>> index 9d5e3c8..d899dac 100644
>> --- a/drivers/gpu/drm/drm_atomic.c
>> +++ b/drivers/gpu/drm/drm_atomic.c
>> @@ -1159,7 +1159,7 @@ drm_atomic_set_crtc_for_connector(struct drm_connector_state *conn_state,
>>                                 struct drm_crtc *crtc)
>>  {
>>       struct drm_crtc_state *crtc_state;
>> -
>> +     bool had_crtc = conn_state->crtc ? true : false;
>>       if (conn_state->crtc && conn_state->crtc != crtc) {
>>               crtc_state = drm_atomic_get_existing_crtc_state(conn_state->state,
>>                                                               conn_state->crtc);
>> @@ -1179,6 +1179,15 @@ drm_atomic_set_crtc_for_connector(struct drm_connector_state *conn_state,
>>
>>       conn_state->crtc = crtc;
>>
>> +     /* If we had no crtc then got one, add a reference,
>> +      * if we had a crtc and are going to none, drop a reference,
>> +      * otherwise just keep the reference we have.
>> +      */
>> +     if (!had_crtc && crtc)
>> +             drm_connector_reference(conn_state->connector);
>> +     else if (!crtc && had_crtc)
>> +              drm_connector_unreference(conn_state->connector);
>> +
>>       if (crtc)
>>               DRM_DEBUG_ATOMIC("Link connector state %p to [CRTC:%d:%s]\n",
>>                                conn_state, crtc->base.id, crtc->name);
>
> I'm super paranoid about the refcounting for modesets, since at least with
> fbs that was where we had endless amounts of pain and random bugs.
>
> - I think we should split this patch into atomic and legacy parts, since
>   the code-paths are completely different.
>
> - I'm not sure on the atomic version. I think conceptually it would be
>   even cleaner for atomic to say that drm_connector_state holds a ref on
>   the connector iff the crtc pointer is set. This would mean we grab the
>   reference also in duplicate_state and drop it in destroy_state (and ofc
>   update it in set_crtc_for_connector). It's a bit funny semantics, but by
>   attaching the refcounting to the lifetime of the state struct we fully
>   align with how refcounting is done for everything else in atomic. And
>   avoiding special cases in refcounting is good imo.
>
>   Also since we know that the state structures are tracked correctly I
>   wouldn't have to think about correctness at all, makes the review easier
>   ;-)

I'm okay on splitting things, just not sure what you want the atomic
thing to look like.

I'll repost a bit of the series and you can tell me if you agree!

Dave.