> -----Original Message-----
> From: Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx>
> Sent: Monday, October 9, 2023 3:37 PM
> To: Kandpal, Suraj <suraj.kandpal@xxxxxxxxx>; intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> Subject: Re: [PATCH v5 1/2] drm/i915/hdcp: Move checks for gsc
> health status
>
> On Mon, 09 Oct 2023, Suraj Kandpal <suraj.kandpal@xxxxxxxxx> wrote:
> > Move checks for gsc components required for HDCP 2.2 to work into
> > intel_hdcp_gsc.c. This will also help with XE refactor on HDCP's side.
> >
> > Signed-off-by: Suraj Kandpal <suraj.kandpal@xxxxxxxxx>
> > Reviewed-by: Uma Shankar <uma.shankar@xxxxxxxxx>
> > ---
> > drivers/gpu/drm/i915/display/intel_hdcp.c | 8 +-------
> > drivers/gpu/drm/i915/display/intel_hdcp_gsc.c | 14 ++++++++++++++
> > drivers/gpu/drm/i915/display/intel_hdcp_gsc.h | 1 +
> > 3 files changed, 16 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_hdcp.c
> > b/drivers/gpu/drm/i915/display/intel_hdcp.c
> > index 8cca4793cf92..c89da3568ebd 100644
> > --- a/drivers/gpu/drm/i915/display/intel_hdcp.c
> > +++ b/drivers/gpu/drm/i915/display/intel_hdcp.c
> > @@ -173,14 +173,8 @@ bool intel_hdcp2_capable(struct intel_connector
> > *connector)
> >
> > /* If MTL+ make sure gsc is loaded and proxy is setup */
> > if (intel_hdcp_gsc_cs_required(i915)) {
> > - struct intel_gt *gt = i915->media_gt;
> > - struct intel_gsc_uc *gsc = gt ? >->uc.gsc : NULL;
> > -
> > - if (!gsc || !intel_uc_fw_is_running(&gsc->fw)) {
> > - drm_dbg_kms(&i915->drm,
> > - "GSC components required for HDCP2.2 are
> not ready\n");
> > + if (!intel_hdcp_gsc_check_status(i915))
>
> Naming nitpick, though not intended to block the patch:
>
> I always stop at functions named "check" that return a value. What does the
> return value mean? The above code reads, "if not check status".
>
> So I usually try to name functions so that it's obvious on both sides what it
> means, when you read it aloud. For example, "if not status ok".
>
> Just something to consider for the future.
>
>
Got it Jani will keep this in mind in future
Regards,
Suraj Kandpal
> BR,
> Jani.
>
>
>
> > return false;
> > - }
> > }
> >
> > /* MEI/GSC interface is solid depending on which is used */ diff
> > --git a/drivers/gpu/drm/i915/display/intel_hdcp_gsc.c
> > b/drivers/gpu/drm/i915/display/intel_hdcp_gsc.c
> > index d753db3eef15..d355d610bc9f 100644
> > --- a/drivers/gpu/drm/i915/display/intel_hdcp_gsc.c
> > +++ b/drivers/gpu/drm/i915/display/intel_hdcp_gsc.c
> > @@ -17,6 +17,20 @@ bool intel_hdcp_gsc_cs_required(struct
> drm_i915_private *i915)
> > return DISPLAY_VER(i915) >= 14;
> > }
> >
> > +bool intel_hdcp_gsc_check_status(struct drm_i915_private *i915) {
> > + struct intel_gt *gt = i915->media_gt;
> > + struct intel_gsc_uc *gsc = gt ? >->uc.gsc : NULL;
> > +
> > + if (!gsc || !intel_uc_fw_is_running(&gsc->fw)) {
> > + drm_dbg_kms(&i915->drm,
> > + "GSC components required for HDCP2.2 are not
> ready\n");
> > + return false;
> > + }
> > +
> > + return true;
> > +}
> > +
> > static int
> > gsc_hdcp_initiate_session(struct device *dev, struct hdcp_port_data *data,
> > struct hdcp2_ake_init *ake_data) diff --git
> > a/drivers/gpu/drm/i915/display/intel_hdcp_gsc.h
> > b/drivers/gpu/drm/i915/display/intel_hdcp_gsc.h
> > index cbf96551e534..eba2057c5a9e 100644
> > --- a/drivers/gpu/drm/i915/display/intel_hdcp_gsc.h
> > +++ b/drivers/gpu/drm/i915/display/intel_hdcp_gsc.h
> > @@ -23,5 +23,6 @@ ssize_t intel_hdcp_gsc_msg_send(struct
> drm_i915_private *i915, u8 *msg_in,
> > size_t msg_out_len);
> > int intel_hdcp_gsc_init(struct drm_i915_private *i915); void
> > intel_hdcp_gsc_fini(struct drm_i915_private *i915);
> > +bool intel_hdcp_gsc_check_status(struct drm_i915_private *i915);
> >
> > #endif /* __INTEL_HDCP_GCS_H__ */
>
> --
> Jani Nikula, Intel
