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

[Nautiyal, Ankit K]

On 3/25/2024 8:48 PM, Jani Nikula wrote:
On Fri, 22 Mar 2024, Ankit Nautiyal <ankit.k.nauti...@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.nauti...@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.

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);