>
> On Wed, 2022-12-14 at 14:37 +0530, Suraj Kandpal wrote:
> > Add function that takes care of sending command to gsc cs. We start of
> > with allocation of memory for our command intel_hdcp_gsc_message that
> > contains gsc cs memory header as directed in specs followed by the
> > actual payload hdcp message that we want to send.
> > Spec states that we need to poll pending bit of response header around
> > 20 times each try being 50ms apart hence adding that to current
> > gsc_msg_send function Also we use the same function to take care of
> > both sending and receiving hence no separate function to get the
> > response.
> >
> > Cc: Ankit Nautiyal <ankit.k.nautiyal@xxxxxxxxx>
> > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@xxxxxxxxx>
> > Cc: Uma Shankar <uma.shankar@xxxxxxxxx>
> > Cc: Anshuman Gupta <anshuman.gupta@xxxxxxxxx>
> > Signed-off-by: Suraj Kandpal <suraj.kandpal@xxxxxxxxx>
> > ---
> > drivers/gpu/drm/i915/Makefile | 1 +
> > drivers/gpu/drm/i915/display/intel_hdcp_gsc.c | 207
> > ++++++++++++++++++ drivers/gpu/drm/i915/display/intel_hdcp_gsc.h | 18
> ++
> > drivers/gpu/drm/i915/gt/uc/intel_gsc_fwif.h | 1 +
> > 4 files changed, 227 insertions(+)
> > create mode 100644 drivers/gpu/drm/i915/display/intel_hdcp_gsc.c
> > create mode 100644 drivers/gpu/drm/i915/display/intel_hdcp_gsc.h
> >
> > diff --git a/drivers/gpu/drm/i915/Makefile
> > b/drivers/gpu/drm/i915/Makefile index f64a8bc73c89..9cae0c1598a7
> > 100644
> > --- a/drivers/gpu/drm/i915/Makefile
> > +++ b/drivers/gpu/drm/i915/Makefile
> > @@ -251,6 +251,7 @@ i915-y += \
> > display/intel_frontbuffer.o \
> > display/intel_global_state.o \
> > display/intel_hdcp.o \
> > + display/intel_hdcp_gsc.o \
> > display/intel_hotplug.o \
> > display/intel_hti.o \
> > display/intel_lpe_audio.o \
> > diff --git a/drivers/gpu/drm/i915/display/intel_hdcp_gsc.c
> > b/drivers/gpu/drm/i915/display/intel_hdcp_gsc.c
> > new file mode 100644
> > index 000000000000..6858b6219221
> > --- /dev/null
> > +++ b/drivers/gpu/drm/i915/display/intel_hdcp_gsc.c
> > @@ -0,0 +1,207 @@
> > +// SPDX-License-Identifier: MIT
> > +/*
> > + * Copyright 2021, Intel Corporation.
> > + */
> > +
> > +#include "display/intel_hdcp_gsc.h"
> > +#include "gem/i915_gem_region.h"
> > +#include "gt/uc/intel_gsc_fw.h"
> > +#include "gt/uc/intel_gsc_fwif.h"
> > +#include "i915_drv.h"
> > +#include "i915_utils.h"
> > +
> > +struct intel_hdcp_gsc_message {
> > + struct drm_i915_gem_object *obj;
> > + struct i915_vma *vma;
> > + void *hdcp_cmd;
> > +};
> >
> Alan: see my last set of comments below.
>
> Alan:[snip
> >
> >
>
> > +static int intel_gsc_send_sync(struct drm_i915_private *i915,
> > + struct intel_gsc_mtl_header *header, u64 addr,
> > + size_t msg_out_len)
> > +{
> > + struct intel_gt *gt = i915->media_gt;
> > + int ret;
> > +
> > + header->flags = 0;
> > + ret = intel_gsc_fw_heci_send(>->uc.gsc, addr, header-
> >message_size,
> > + addr, msg_out_len + sizeof(*header));
> > + if (ret) {
> > + drm_err(&i915->drm, "failed to send gsc HDCP msg (%d)\n",
> ret);
> > + return ret;
> > + }
> > + /*
> > + * Checking validity marker for memory sanity
> > + */
> > + if (header->validity_marker != GSC_HECI_VALIDITY_MARKER) {
> > + drm_err(&i915->drm, "invalid validity marker\n");
> > + return -EINVAL;
> > + }
> > +
> > + if (header->status != 0) {
> > + drm_err(&i915->drm, "header status indicates error %d\n",
> > + header->status);
> > + return -EINVAL;
> > + }
> > +
> > + if (header->flags & INTEL_GSC_MSG_PENDING)
> > + return -EAGAIN;
> > +
> > + return 0;
> > +}
> > +
> Alan: As per my comment on patch #1 above function could go into the uc/gsc
> domain ... see the comment below for further elaboration.
>
> > +/*
> > + * This function can now be used for sending requests and will also
> > +handle
> > + * receipt of reply messages hence no different function of message
> > +retrieval
> > + * is required. We will initialize intel_hdcp_gsc_message structure
> > +then add
> > + * gsc cs memory header as stated in specs after which the normal
> > +HDCP payload
> > + * will follow
> > + */
> > +ssize_t intel_hdcp_gsc_msg_send(struct drm_i915_private *i915, u8 *msg_in,
> > + size_t msg_in_len, u8 *msg_out, size_t
> msg_out_len) {
> > + struct intel_gt *gt = i915->media_gt;
> > + struct intel_gsc_mtl_header *header;
> > + const size_t max_msg_size = PAGE_SIZE - sizeof(*header);
> > + struct intel_hdcp_gsc_message *hdcp_message;
> > + u64 addr;
> > + u32 reply_size;
> > + int ret, tries = 0;
> > +
> > + if (!intel_uc_uses_gsc_uc(>->uc))
> > + return -ENODEV;
> > +
> > + if (msg_in_len > max_msg_size || msg_out_len > max_msg_size)
> > + return -ENOSPC;
> > +
> > + hdcp_message = kzalloc(sizeof(*hdcp_message), GFP_KERNEL);
> > +
> > + if (!hdcp_message)
> > + return -ENOMEM;
> > +
> > + ret = intel_initialize_hdcp_gsc_message(i915, hdcp_message);
> > +
> > + if (ret) {
> > + drm_err(&i915->drm,
> > + "Could not initialize hdcp_message\n");
> > + goto err;
> > + }
> > +
> > + header = hdcp_message->hdcp_cmd;
> > + addr = i915_ggtt_offset(hdcp_message->vma);
> > +
> > + memset(header, 0, sizeof(*header));
> > + header->validity_marker = GSC_HECI_VALIDITY_MARKER;
> > + header->gsc_address = HECI_MEADDRESS_HDCP;
> > + header->host_session_handle = 0;
> > + header->header_version = MTL_GSC_HEADER_VERSION;
> > + header->message_size = msg_in_len + sizeof(*header);
> > +
> > + memcpy(hdcp_message->hdcp_cmd + sizeof(*header), msg_in,
> > +msg_in_len);
> > +
> > + /*
> > + * Keep sending request in case the pending bit is set no need to add
> > + * message handle as we are using same address hence loc. of header is
> > + * same and it will contain the message handle. we will send the message
> > + * 20 times each message 50 ms apart
> > + */
> > + do {
> > + ret = intel_gsc_send_sync(i915, header, addr, msg_out_len);
> > +
> > + /* Only try again if gsc says so */
> > + if (ret != -EAGAIN)
> > + break;
> > +
> > + msleep(50);
> > +
> > + } while (++tries < 20);
> > +
> > + if (ret)
> > + goto err;
> > +
> > + /* we use the same mem for the reply, so header is in the same loc */
> > + reply_size = header->message_size - sizeof(*header);
> > + if (reply_size > msg_out_len) {
> > + drm_warn(&i915->drm, "caller with insufficient HDCP reply size
> %u (%d)\n",
> > + reply_size, (u32)msg_out_len);
> > + reply_size = msg_out_len;
> > + } else if (reply_size != msg_out_len) {
> > + drm_dbg_kms(&i915->drm, "caller unexpected HCDP reply size
> %u (%d)\n",
> > + reply_size, (u32)msg_out_len);
> > + }
> > +
> > + memcpy(msg_out, hdcp_message->hdcp_cmd + sizeof(*header),
> > +msg_out_len);
> > +
> > +err:
> > + intel_free_hdcp_gsc_message(hdcp_message);
> > + return ret;
> > +}
>
> Alan: See my review comment on patch #1 - i believe most of this function above
> (intel_hdcp_gsc_msg_send) could go into a common
> intel_gsc_engine_send_hecipkt function (in a new gsc-heci specific file,
> i.e. intel_gsc_heci.c) that lives in the uc/gsc domain, not here in display. In fact
> the "struct intel_hdcp_gsc_message" also also be renamed to be "struct
> intel_gsc_heci_pkt_info" and its definition moved over to (and included from) a
> header in the uc/gsc domain.
> I believe it make sense for the caller to allocate the objects but the common
> header to have the structure definition and the common function can do the
> cmd-prep, submission, waiting (and eventually checking of pending-bit).
>
I can move a lot of these functions to intel_gsc_fw.c
But I still don’t see the merit in adding more functions and files in just for more readability
Regards,
Suraj Kandpal
>
> Alan:[snip]
