Re: [Intel-gfx] [PATCH v2 5/6] drm/atomic: Handle encoder assignment conflicts in a separate check.

From: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
To: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
Cc: intel-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org
Subject: Re: [Intel-gfx] [PATCH v2 5/6] drm/atomic: Handle encoder assignment conflicts in a separate check.
Date: Tue, 1 Mar 2016 18:45:53 +0100	[thread overview]
Message-ID: <56D5D551.2000004@linux.intel.com> (raw)
In-Reply-To: <20160301172119.GL15993@intel.com>

Op 01-03-16 om 18:21 schreef Ville Syrjälä:
> On Wed, Feb 24, 2016 at 09:37:32AM +0100, Maarten Lankhorst wrote:
>> The current check doesn't handle the case where we don't steal an
>> encoder, but keep it on the current connector. If we repurpose
>> disable_conflicting_encoders to do the checking, we just have
>> to reject the ones that conflict.
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> Testcase: kms_setmode.invalid-clone-single-crtc-stealing
>> ---
>>  drivers/gpu/drm/drm_atomic_helper.c | 58 +++++++++++++++----------------------
>>  1 file changed, 24 insertions(+), 34 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
>> index 3543c7fcd072..32bd5bebef0b 100644
>> --- a/drivers/gpu/drm/drm_atomic_helper.c
>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
>> @@ -86,7 +86,8 @@ drm_atomic_helper_plane_changed(struct drm_atomic_state *state,
>>  	}
>>  }
>>  
>> -static int disable_conflicting_connectors(struct drm_atomic_state *state)
>> +static int handle_conflicting_encoders(struct drm_atomic_state *state,
>> +				       bool disable_conflicting_encoders)
>>  {
>>  	struct drm_connector_state *conn_state;
>>  	struct drm_connector *connector;
>> @@ -106,8 +107,17 @@ static int disable_conflicting_connectors(struct drm_atomic_state *state)
>>  		else
>>  			new_encoder = funcs->best_encoder(connector);
>>  
>> -		if (new_encoder)
>> +		if (new_encoder) {
>> +			if (encoder_mask & (1 << drm_encoder_index(new_encoder))) {
>> +				DRM_DEBUG_ATOMIC("[ENCODER:%d:%s] on [CONNECTOR:%d:%s] already assigned\n",
>> +					new_encoder->base.id, new_encoder->name,
>> +					connector->base.id, connector->name);
>> +
>> +				return -EINVAL;
>> +			}
>> +
>>  			encoder_mask |= 1 << drm_encoder_index(new_encoder);
>> +		}
>>  	}
>>  
>>  	drm_for_each_connector(connector, state->dev) {
>> @@ -120,6 +130,15 @@ static int disable_conflicting_connectors(struct drm_atomic_state *state)
>>  		if (!encoder || !(encoder_mask & (1 << drm_encoder_index(encoder))))
>>  			continue;
>>  
>> +		if (!disable_conflicting_encoders) {
>> +			DRM_DEBUG_ATOMIC("[ENCODER:%d:%s] in use on [CRTC:%d:%s] by [CONNECTOR:%d:%s]\n",
>> +					 encoder->base.id, encoder->name,
>> +					 connector->state->crtc->base.id,
>> +					 connector->state->crtc->name,
>> +					 connector->base.id, connector->name);
>> +			return -EINVAL;
>> +		}
>> +
> Hmm. This can't possibly work can it? If I'm reding things correctly
> this would already fail if we have crtc0->enc0->conn0 and then try to
> change it to crtc1->enc0->conn0. But perhaps I'm missing some subtle
> thing (there are a lot of those in our atomic framework due to thing
> automagically getting added to the state).
>
No, in that case the connector is part of the state.

This boils down to:

if (stealing encoder from existing connector not part of state)
if (atomic) return -EINVAL;
else
// disable connector and possibly crtc

~Maarten

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel