On 22 April 2016 at 18:49, Daniel Vetter <daniel@xxxxxxxx> wrote:
> On Fri, Apr 15, 2016 at 03:10:44PM +1000, Dave Airlie wrote:
>> From: Dave Airlie <airlied@xxxxxxxxxx>
>>
>> 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@xxxxxxxxxx>
>> ---
>> 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.
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
