[PATCH 2/2] drm/i915/lspcon: Separate lspcon probe and lspcon init

[Nautiyal, Ankit K]

On 3/26/2024 12:26 PM, Jani Nikula wrote:
> On Tue, 26 Mar 2024, "Nautiyal, Ankit K" <ankit.k.nautiyal at intel.com> wrote:
>> On 3/25/2024 8:48 PM, Jani Nikula wrote:
>>> On Fri, 22 Mar 2024, Ankit Nautiyal <ankit.k.nautiyal at intel.com> wrote:
>>>> Currently we probe for lspcon, inside lspcon init. Which does 2 things:
>>>> probe the lspcon and set the expected LS/PCON mode.
>>>>
>>>> If there is no lspcon connected, the probe expectedly fails and
>>>> results in error message. This inturn gets propogated to
>>>> lspcon init and we get again error message for lspcon init
>>>> failure.
>>>>
>>>> Separate the probe function and avoid displaying error if probe fails.
>>>> If probe succeeds, only then start lspcon init and set the expected
>>>> LS/PCON mode as first step.
>>>>
>>>> While at it move the drm_err message in lspcon init, instead of the
>>>> caller.
>>>>
>>>> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal at intel.com>
>>>> ---
>>>>    drivers/gpu/drm/i915/display/intel_dp.c     |  3 +++
>>>>    drivers/gpu/drm/i915/display/intel_lspcon.c | 27 +++++++++++----------
>>>>    drivers/gpu/drm/i915/display/intel_lspcon.h |  1 +
>>>>    3 files changed, 18 insertions(+), 13 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
>>>> index 94fa34f77cf0..ea8d3e70127e 100644
>>>> --- a/drivers/gpu/drm/i915/display/intel_dp.c
>>>> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
>>>> @@ -5882,6 +5882,9 @@ intel_dp_connector_register(struct drm_connector *connector)
>>>>    	 * ToDo: Clean this up to handle lspcon init and resume more
>>>>    	 * efficiently and streamlined.
>>>>    	 */
>>>> +	if (!lspcon_probe(lspcon))
>>>> +		return ret;
>>>> +
>>>>    	if (lspcon_init(dig_port)) {
>>>>    		lspcon_detect_hdr_capability(lspcon);
>>>>    		if (lspcon->hdr_supported)
>>>> diff --git a/drivers/gpu/drm/i915/display/intel_lspcon.c b/drivers/gpu/drm/i915/display/intel_lspcon.c
>>>> index 62159d3ead56..570fde848d00 100644
>>>> --- a/drivers/gpu/drm/i915/display/intel_lspcon.c
>>>> +++ b/drivers/gpu/drm/i915/display/intel_lspcon.c
>>>> @@ -266,7 +266,7 @@ static bool lspcon_set_expected_mode(struct intel_lspcon *lspcon)
>>>>    	return true;
>>>>    }
>>>>    
>>>> -static bool lspcon_probe(struct intel_lspcon *lspcon)
>>>> +bool lspcon_probe(struct intel_lspcon *lspcon)
>>>>    {
>>>>    	int retry;
>>>>    	enum drm_dp_dual_mode_type adaptor_type;
>>>> @@ -676,30 +676,31 @@ bool lspcon_init(struct intel_digital_port *dig_port)
>>>>    	lspcon->active = false;
>>>>    	lspcon->mode = DRM_LSPCON_MODE_INVALID;
>>>>    
>>>> -	if (!lspcon_probe(lspcon)) {
>>>> -		drm_err(&i915->drm, "Failed to probe lspcon\n");
>>>> -		return false;
>>>> -	}
>>>> -
>>>>    	if (!lspcon_set_expected_mode(lspcon)) {
>>>>    		drm_err(&i915->drm, "LSPCON Set expected Mode failed\n");
>>>> -		return false;
>>>> +		goto lspcon_init_failed;
>>>>    	}
>>>>    
>>>>    	if (drm_dp_read_dpcd_caps(&intel_dp->aux, intel_dp->dpcd) != 0) {
>>>>    		drm_err(&i915->drm, "LSPCON DPCD read failed\n");
>>>> -		return false;
>>>> +		goto lspcon_init_failed;
>>>>    	}
>>>>    
>>>>    	if (!lspcon_detect_vendor(lspcon)) {
>>>>    		drm_err(&i915->drm, "LSPCON vendor detection failed\n");
>>>> -		return false;
>>>> +		goto lspcon_init_failed;
>>>>    	}
>>>>    
>>>>    	connector->ycbcr_420_allowed = true;
>>>>    	lspcon->active = true;
>>>>    	drm_dbg_kms(&i915->drm, "Success: LSPCON init\n");
>>>>    	return true;
>>>> +
>>>> +lspcon_init_failed:
>>>> +	drm_err(&i915->drm, "LSPCON init failed on port %c\n",
>>>> +		port_name(dig_port->base.port));
>>> I guess the idea is to reduce dmesg errors, but in this function the
>>> error messages are multiplied.
>> Earlier we used to get the drm_error for init failure, even if the
>> LSPCON was not detected, which is printed as a debug print.
>>
>> Now we'll get the dmesg errors only if we detect lspcon and lspcon init
>> indeed fails.
> I was referring to lspcon_probe() which now has drm_err() on each of the
> branches which goto lspcon_init_failed, which has another
> drm_err(). There's no need for the extra error message.

Ah ok, I got it, I can get rid of extra error message at the end.

Thanks & Regards,

Ankit

> BR,
> Jani.
>
>
>> Regards,
>>
>> Ankit
>>
>>
>>> BR,
>>> Jani.
>>>
>>>> +
>>>> +	return false;
>>>>    }
>>>>    
>>>>    u32 intel_lspcon_infoframes_enabled(struct intel_encoder *encoder,
>>>> @@ -721,11 +722,11 @@ void lspcon_resume(struct intel_digital_port *dig_port)
>>>>    		return;
>>>>    
>>>>    	if (!lspcon->active) {
>>>> -		if (!lspcon_init(dig_port)) {
>>>> -			drm_err(&i915->drm, "LSPCON init failed on port %c\n",
>>>> -				port_name(dig_port->base.port));
>>>> +		if (!lspcon_probe(lspcon))
>>>> +			return;
>>>> +
>>>> +		if (!lspcon_init(dig_port))
>>>>    			return;
>>>> -		}
>>>>    	}
>>>>    
>>>>    	if (lspcon_wake_native_aux_ch(lspcon)) {
>>>> diff --git a/drivers/gpu/drm/i915/display/intel_lspcon.h b/drivers/gpu/drm/i915/display/intel_lspcon.h
>>>> index e19e10492b05..b156cc6b3a23 100644
>>>> --- a/drivers/gpu/drm/i915/display/intel_lspcon.h
>>>> +++ b/drivers/gpu/drm/i915/display/intel_lspcon.h
>>>> @@ -16,6 +16,7 @@ struct intel_encoder;
>>>>    struct intel_lspcon;
>>>>    
>>>>    bool lspcon_init(struct intel_digital_port *dig_port);
>>>> +bool lspcon_probe(struct intel_lspcon *lspcon);
>>>>    void lspcon_detect_hdr_capability(struct intel_lspcon *lspcon);
>>>>    void lspcon_resume(struct intel_digital_port *dig_port);
>>>>    void lspcon_wait_pcon_mode(struct intel_lspcon *lspcon);