Re: [Intel-gfx] [PATCH 2/2] drm/i915/display: throw out struct intel_load_detect_pipe

[Jani Nikula <jani.nikula@intel.com>]

On Tue, 18 Apr 2023, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote:
> On Mon, Apr 17, 2023 at 06:37:41PM +0300, Jani Nikula wrote:
>> An error-valued pointer can handle all in one without the wrapper
>> struct.
>> 
>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> ---
>>  drivers/gpu/drm/i915/display/intel_crt.c      | 18 ++++++++---------
>>  .../gpu/drm/i915/display/intel_load_detect.c  | 20 ++++++++-----------
>>  .../gpu/drm/i915/display/intel_load_detect.h  | 12 ++++-------
>>  drivers/gpu/drm/i915/display/intel_tv.c       | 16 +++++++--------
>>  4 files changed, 29 insertions(+), 37 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/display/intel_crt.c b/drivers/gpu/drm/i915/display/intel_crt.c
>> index 96acdf98a0c0..13519f78cf9f 100644
>> --- a/drivers/gpu/drm/i915/display/intel_crt.c
>> +++ b/drivers/gpu/drm/i915/display/intel_crt.c
>> @@ -822,9 +822,9 @@ intel_crt_detect(struct drm_connector *connector,
>>  	struct drm_i915_private *dev_priv = to_i915(connector->dev);
>>  	struct intel_crt *crt = intel_attached_crt(to_intel_connector(connector));
>>  	struct intel_encoder *intel_encoder = &crt->base;
>> +	struct drm_atomic_state *state;
>>  	intel_wakeref_t wakeref;
>> -	int status, ret;
>> -	struct intel_load_detect_pipe tmp;
>> +	int status;
>>  
>>  	drm_dbg_kms(&dev_priv->drm, "[CONNECTOR:%d:%s] force=%d\n",
>>  		    connector->base.id, connector->name,
>> @@ -882,8 +882,12 @@ intel_crt_detect(struct drm_connector *connector,
>>  	}
>>  
>>  	/* for pre-945g platforms use load detect */
>> -	ret = intel_load_detect_get_pipe(connector, &tmp, ctx);
>> -	if (ret > 0) {
>> +	state = intel_load_detect_get_pipe(connector, ctx);
>> +	if (IS_ERR(state)) {
>> +		status = PTR_ERR(state);
>> +	} else if (!state) {
>> +		status = connector_status_unknown;
>> +	} else {
>>  		if (intel_crt_detect_ddc(connector))
>>  			status = connector_status_connected;
>>  		else if (DISPLAY_VER(dev_priv) < 4)
>> @@ -893,11 +897,7 @@ intel_crt_detect(struct drm_connector *connector,
>>  			status = connector_status_disconnected;
>>  		else
>>  			status = connector_status_unknown;
>> -		intel_load_detect_release_pipe(connector, &tmp, ctx);
>> -	} else if (ret == 0) {
>> -		status = connector_status_unknown;
>> -	} else {
>> -		status = ret;
>> +		intel_load_detect_release_pipe(connector, state, ctx);
>
> I confess it took me a while to see that we have the same logic in place.
> I think I need more coffee.

It also took me a while when writing it! :)

> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

Thanks, both pushed to din.

BR,
Jani.

>
>>  	}
>>  
>>  out:
>> diff --git a/drivers/gpu/drm/i915/display/intel_load_detect.c b/drivers/gpu/drm/i915/display/intel_load_detect.c
>> index 5d6bb6d712bc..d5a0aecf3e8f 100644
>> --- a/drivers/gpu/drm/i915/display/intel_load_detect.c
>> +++ b/drivers/gpu/drm/i915/display/intel_load_detect.c
>> @@ -44,9 +44,9 @@ static int intel_modeset_disable_planes(struct drm_atomic_state *state,
>>  	return 0;
>>  }
>>  
>> -int intel_load_detect_get_pipe(struct drm_connector *connector,
>> -			       struct intel_load_detect_pipe *old,
>> -			       struct drm_modeset_acquire_ctx *ctx)
>> +struct drm_atomic_state *
>> +intel_load_detect_get_pipe(struct drm_connector *connector,
>> +			   struct drm_modeset_acquire_ctx *ctx)
>>  {
>>  	struct intel_encoder *encoder =
>>  		intel_attached_encoder(to_intel_connector(connector));
>> @@ -64,8 +64,6 @@ int intel_load_detect_get_pipe(struct drm_connector *connector,
>>  		    connector->base.id, connector->name,
>>  		    encoder->base.base.id, encoder->base.name);
>>  
>> -	old->restore_state = NULL;
>> -
>>  	drm_WARN_ON(dev, !drm_modeset_is_locked(&config->connection_mutex));
>>  
>>  	/*
>> @@ -179,13 +177,12 @@ int intel_load_detect_get_pipe(struct drm_connector *connector,
>>  		goto fail;
>>  	}
>>  
>> -	old->restore_state = restore_state;
>>  	drm_atomic_state_put(state);
>>  
>>  	/* let the connector get through one full cycle before testing */
>>  	intel_crtc_wait_for_next_vblank(crtc);
>>  
>> -	return true;
>> +	return restore_state;
>>  
>>  fail:
>>  	if (state) {
>> @@ -198,27 +195,26 @@ int intel_load_detect_get_pipe(struct drm_connector *connector,
>>  	}
>>  
>>  	if (ret == -EDEADLK)
>> -		return ret;
>> +		return ERR_PTR(ret);
>>  
>> -	return false;
>> +	return NULL;
>>  }
>>  
>>  void intel_load_detect_release_pipe(struct drm_connector *connector,
>> -				    struct intel_load_detect_pipe *old,
>> +				    struct drm_atomic_state *state,
>>  				    struct drm_modeset_acquire_ctx *ctx)
>>  {
>>  	struct intel_encoder *intel_encoder =
>>  		intel_attached_encoder(to_intel_connector(connector));
>>  	struct drm_i915_private *i915 = to_i915(intel_encoder->base.dev);
>>  	struct drm_encoder *encoder = &intel_encoder->base;
>> -	struct drm_atomic_state *state = old->restore_state;
>>  	int ret;
>>  
>>  	drm_dbg_kms(&i915->drm, "[CONNECTOR:%d:%s], [ENCODER:%d:%s]\n",
>>  		    connector->base.id, connector->name,
>>  		    encoder->base.id, encoder->name);
>>  
>> -	if (!state)
>> +	if (IS_ERR_OR_NULL(state))
>>  		return;
>>  
>>  	ret = drm_atomic_helper_commit_duplicated_state(state, ctx);
>> diff --git a/drivers/gpu/drm/i915/display/intel_load_detect.h b/drivers/gpu/drm/i915/display/intel_load_detect.h
>> index 9b69da1867a5..aed51901b9ba 100644
>> --- a/drivers/gpu/drm/i915/display/intel_load_detect.h
>> +++ b/drivers/gpu/drm/i915/display/intel_load_detect.h
>> @@ -10,15 +10,11 @@ struct drm_atomic_state;
>>  struct drm_connector;
>>  struct drm_modeset_acquire_ctx;
>>  
>> -struct intel_load_detect_pipe {
>> -	struct drm_atomic_state *restore_state;
>> -};
>> -
>> -int intel_load_detect_get_pipe(struct drm_connector *connector,
>> -			       struct intel_load_detect_pipe *old,
>> -			       struct drm_modeset_acquire_ctx *ctx);
>> +struct drm_atomic_state *
>> +intel_load_detect_get_pipe(struct drm_connector *connector,
>> +			   struct drm_modeset_acquire_ctx *ctx);
>>  void intel_load_detect_release_pipe(struct drm_connector *connector,
>> -				    struct intel_load_detect_pipe *old,
>> +				    struct drm_atomic_state *old,
>>  				    struct drm_modeset_acquire_ctx *ctx);
>>  
>>  #endif /* __INTEL_LOAD_DETECT_H__ */
>> diff --git a/drivers/gpu/drm/i915/display/intel_tv.c b/drivers/gpu/drm/i915/display/intel_tv.c
>> index 07e7f7cdd961..e3ccface0c9d 100644
>> --- a/drivers/gpu/drm/i915/display/intel_tv.c
>> +++ b/drivers/gpu/drm/i915/display/intel_tv.c
>> @@ -1723,21 +1723,21 @@ intel_tv_detect(struct drm_connector *connector,
>>  		return connector_status_disconnected;
>>  
>>  	if (force) {
>> -		struct intel_load_detect_pipe tmp;
>> -		int ret;
>> +		struct drm_atomic_state *state;
>>  
>> -		ret = intel_load_detect_get_pipe(connector, &tmp, ctx);
>> -		if (ret < 0)
>> -			return ret;
>> +		state = intel_load_detect_get_pipe(connector, ctx);
>> +		if (IS_ERR(state))
>> +			return PTR_ERR(state);
>>  
>> -		if (ret > 0) {
>> +		if (state) {
>>  			type = intel_tv_detect_type(intel_tv, connector);
>> -			intel_load_detect_release_pipe(connector, &tmp, ctx);
>> +			intel_load_detect_release_pipe(connector, state, ctx);
>>  			status = type < 0 ?
>>  				connector_status_disconnected :
>>  				connector_status_connected;
>> -		} else
>> +		} else {
>>  			status = connector_status_unknown;
>> +		}
>>  
>>  		if (status == connector_status_connected) {
>>  			intel_tv->type = type;
>> -- 
>> 2.39.2
>> 

-- 
Jani Nikula, Intel Open Source Graphics Center