On Tue, 26 Mar 2024, "Nautiyal, Ankit K" <ankit.k.nautiyal@xxxxxxxxx> wrote:
> On 3/25/2024 8:48 PM, Jani Nikula wrote:
>> On Fri, 22 Mar 2024, Ankit Nautiyal <ankit.k.nautiyal@xxxxxxxxx> 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@xxxxxxxxx>
>>> ---
>>> 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.
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);
--
Jani Nikula, Intel
