Re: [PATCH] drm/xe/hdcp: Add check to remove hdcp2 compatibility

[Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>]

On 10/24/2024 6:21 PM, Kandpal, Suraj wrote:
>
>> -----Original Message-----
>> From: Ceraolo Spurio, Daniele <daniele.ceraolospurio@intel.com>
>> Sent: Thursday, October 24, 2024 9:03 PM
>> To: Kandpal, Suraj <suraj.kandpal@intel.com>; intel-xe@lists.freedesktop.org;
>> intel-gfx@lists.freedesktop.org
>> Cc: Nautiyal, Ankit K <ankit.k.nautiyal@intel.com>; Ghimiray, Himal Prasad
>> <himal.prasad.ghimiray@intel.com>
>> Subject: Re: [PATCH] drm/xe/hdcp: Add check to remove hdcp2 compatibility
>>
>>
>>
>>
>> On 10/22/2024 12:29 AM, Suraj Kandpal wrote:
>>> Add check to remove HDCP2 compatibility from BMG as it does not have
>>> GSC which ends up causing warning when we try to get reference of GSC
>>> FW.
>>>
>>> Fixes: 89d030804831 ("drm/xe/hdcp: Fix condition for hdcp gsc cs
>>> requirement")
>>> Fixes: 883631771038 ("drm/i915/mtl: Add HDCP GSC interface")
>>> Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
>>> Reviewed-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
>>> Reviewed-by: Himal Prasad Ghimiray <himal.prasad.ghimiray@intel.com>
>>> ---
>>>    drivers/gpu/drm/i915/display/intel_hdcp_gsc.c | 3 ++-
>>>    drivers/gpu/drm/xe/display/xe_hdcp_gsc.c      | 4 +++-
>>>    2 files changed, 5 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/display/intel_hdcp_gsc.c
>>> b/drivers/gpu/drm/i915/display/intel_hdcp_gsc.c
>>> index 55965844d829..2c1d0ee8cec2 100644
>>> --- a/drivers/gpu/drm/i915/display/intel_hdcp_gsc.c
>>> +++ b/drivers/gpu/drm/i915/display/intel_hdcp_gsc.c
>>> @@ -21,7 +21,8 @@ struct intel_hdcp_gsc_message {
>>>
>>>    bool intel_hdcp_gsc_cs_required(struct intel_display *display)
>>>    {
>>> -	return DISPLAY_VER(display) >= 14;
>>> +	return DISPLAY_VER(display) >= 14 &&
>>> +		DISPLAY_VER_FULL(display) != IP_VER(14, 1);
>>>    }
>>>
>>>    bool intel_hdcp_gsc_check_status(struct intel_display *display) diff
>>> --git a/drivers/gpu/drm/xe/display/xe_hdcp_gsc.c
>>> b/drivers/gpu/drm/xe/display/xe_hdcp_gsc.c
>>> index 231677129a35..efa3441c249c 100644
>>> --- a/drivers/gpu/drm/xe/display/xe_hdcp_gsc.c
>>> +++ b/drivers/gpu/drm/xe/display/xe_hdcp_gsc.c
>>> @@ -8,6 +8,7 @@
>>>    #include <linux/delay.h>
>>>
>>>    #include "abi/gsc_command_header_abi.h"
>>> +#include "i915_drv.h"
>>>    #include "intel_hdcp_gsc.h"
>>>    #include "intel_hdcp_gsc_message.h"
>>>    #include "xe_bo.h"
>>> @@ -32,7 +33,8 @@ struct intel_hdcp_gsc_message {
>>>
>>>    bool intel_hdcp_gsc_cs_required(struct intel_display *display)
>>>    {
>>> -	return DISPLAY_VER(display) >= 14;
>>> +	return DISPLAY_VER(display) >= 14 &&
>>> +		DISPLAY_VER_FULL(display) != IP_VER(14, 1);
>> I don't think this is the correct check or the correct location. BMG does
>> require the GSC for HDCP, so intel_hdcp_gsc_cs_required() should still return
>> true; it's just that we've decided not to support GSC FW loading on the
>> platform, so we can't support HDCP2.x. Also note that the this might change
>> and/or it might apply to other platform in the future, so any check needs to
>> be done based on GSC support and not platform/display ID.
>>
>> IMO when intel_hdcp_gsc_cs_required() returns true, the caller should check
>> if the GSC FW is defined (or if the GSCCS is available) and if it is not return
>> that hdcp2 is not supported due to unmet prerequsites and fallback to 1.4
>> without printing any errors.
>>
> Here is the thing before this I thought that should have worked too but after hdcp_gsc_cs_required()
> We call intel_hdcp_gsc_check_status() which has the following check
>
> if (!gsc && !xe_uc_fw_is_enabled(&gsc->fw)) {

This check seems incorrect to me. Shouldn't it be an OR instead of an 
AND? It is an OR in the i915 code.

>                  drm_dbg_kms(&xe->drm,
>                              "GSC Components not ready for HDCP2.x\n");
>                  return false;
>   }
>
> And this should have returned from here but it does not it goes ahead and tries to get a xe_pm_runtime()
> Which causes it to shout out loud which is currently causing a lot of noise in CI

See comment above about possible issue. But even if that is not the bug, 
if this function should return and it is not then we should fix this, 
not hack the intel_hdcp_gsc_cs_required() function.

Daniele

>
> Regards,
> Suraj Kandpal
>
>> Daniele
>>
>>>    }
>>>
>>>    bool intel_hdcp_gsc_check_status(struct intel_display *display)