Re: [PATCH 3/3] drm/915/lspcon: Reduce dmesg errors during lspcon_init failure

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

On Mon, 08 Apr 2024, Ankit Nautiyal <ankit.k.nautiyal@intel.com> wrote:
> Currently lspcon_resume calls lspcon_init and in case of failure we get
> error messages from lspcon_init and then again from lspcon_resume.
>
> Just have a single error message in lspcon_init and convert all other
> errors as dbg messages.
>
> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_lspcon.c | 27 +++++++++++----------
>  1 file changed, 14 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_lspcon.c b/drivers/gpu/drm/i915/display/intel_lspcon.c
> index 16ee0dc179f7..3c3bc80e32f0 100644
> --- a/drivers/gpu/drm/i915/display/intel_lspcon.c
> +++ b/drivers/gpu/drm/i915/display/intel_lspcon.c
> @@ -680,24 +680,30 @@ bool lspcon_init(struct intel_digital_port *dig_port)
>  		return false;
>  
>  	if (!lspcon_set_pcon_mode(lspcon)) {
> -		drm_err(&i915->drm, "LSPCON mode change to PCON failed\n");
> -		return false;
> +		drm_dbg_kms(&i915->drm, "LSPCON mode change to PCON failed\n");
> +		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;
> +		drm_dbg_kms(&i915->drm, "LSPCON DPCD read failed\n");
> +		goto lspcon_init_failed;
>  	}
>  
>  	if (!lspcon_detect_vendor(lspcon)) {
> -		drm_err(&i915->drm, "LSPCON vendor detection failed\n");
> -		return false;
> +		drm_dbg_kms(&i915->drm, "LSPCON vendor detection failed\n");
> +		goto lspcon_init_failed;

Why not just keep all of the above as drm_err(), adding all the relevant
info there...

>  	}
>  
>  	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));
> +
> +	return false;

And dropping this change altogheter? Why would we need both the debug
message and the error? Just have one error message?

With that change,

Reviewed-by: Jani Nikula <jani.nikula@intel.com>


>  }
>  
>  u32 intel_lspcon_infoframes_enabled(struct intel_encoder *encoder,
> @@ -718,13 +724,8 @@ void lspcon_resume(struct intel_digital_port *dig_port)
>  	if (!intel_bios_encoder_is_lspcon(dig_port->base.devdata))
>  		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));
> -			return;
> -		}
> -	}
> +	if (!lspcon->active && !lspcon_init(dig_port))
> +		return;
>  
>  	expected_mode = lspcon_get_expected_mode(lspcon);
>  	if (expected_mode == DRM_LSPCON_MODE_PCON)

-- 
Jani Nikula, Intel