Re: [Intel-gfx] [PATCH v4 5/7] drm/i915/hdcp: Fill wired_cmd_in structures at a single place
>
> On Thu, 22 Dec 2022, Suraj Kandpal wrote:
> > Need to fill wired cmd in structures at a single place as they remain
> > same for both gsc and mei.
>
> It's probably a good idea to have them in a single place, but there's really
> no
> reason for that place to be part of i915 at all.
>
> None of the functions are about i915.
>
> Feels weird to have MEI HDCP call back to i915. It's supposed to be the glue
> layer
> between i915 and ME FW. Maybe the abstractions are wrong?
So here none of the functions are being called by i915 but going forward if you
refer to patch 7
In this series intel_hdcp_gsc.c has functions that call these fillers hence I
decided to keep this a part
of i915
Regards,
Suraj Kandpal
>
> We currently have a total of 5 exported symbols, and I'd prefer to keep it
> that
> way.
>
>
> BR,
> Jani.
>
>
> >
> > --v3
> > -remove inline function from header [Jani]
> >
> > Cc: Ankit Nautiyal
> > Signed-off-by: Suraj Kandpal
> > ---
> > drivers/gpu/drm/i915/Makefile | 1 +
> > drivers/gpu/drm/i915/i915_hdcp_interface.c | 216 +
> > drivers/misc/mei/hdcp/mei_hdcp.c | 153 ++-
> > include/drm/i915_hdcp_interface.h | 39
> > 4 files changed, 270 insertions(+), 139 deletions(-) create mode
> > 100644 drivers/gpu/drm/i915/i915_hdcp_interface.c
> >
> > diff --git a/drivers/gpu/drm/i915/Makefile
> > b/drivers/gpu/drm/i915/Makefile index 5b5df71cb5d7..b0b2a47455df
> > 100644
> > --- a/drivers/gpu/drm/i915/Makefile
> > +++ b/drivers/gpu/drm/i915/Makefile
> > @@ -36,6 +36,7 @@ i915-y += i915_driver.o \
> > i915_drm_client.o \
> > i915_config.o \
> > i915_getparam.o \
> > + i915_hdcp_interface.o\
> > i915_ioctl.o \
> > i915_irq.o \
> > i915_mitigations.o \
> > diff --git a/drivers/gpu/drm/i915/i915_hdcp_interface.c
> > b/drivers/gpu/drm/i915/i915_hdcp_interface.c
> > new file mode 100644
> > index ..e6b787c2fa50
> > --- /dev/null
> > +++ b/drivers/gpu/drm/i915/i915_hdcp_interface.c
> > @@ -0,0 +1,216 @@
> > +// SPDX-License-Identifier: MIT
> > +/*
> > + * Copyright 2022, Intel Corporation.
> > + */
> > +
> > +#include
> > +
> > +void
> > +i915_hdcp_fill_session_in(struct wired_cmd_initiate_hdcp2_session_in
> *session_init_in,
> > + struct hdcp_port_data *data)
> > +{
> > + session_init_in->header.api_version = HDCP_API_VERSION;
> > + session_init_in->header.command_id =
> WIRED_INITIATE_HDCP2_SESSION;
> > + session_init_in->header.status = FW_HDCP_STATUS_SUCCESS;
> > + session_init_in->header.buffer_len =
> > +
> WIRED_CMD_BUF_LEN_INITIATE_HDCP2_SESSION_IN;
> > +
> > + session_init_in->port.integrated_port_type = data->port_type;
> > + session_init_in->port.physical_port = (u8)data->hdcp_ddi;
> > + session_init_in->port.attached_transcoder = (u8)data-
> >hdcp_transcoder;
> > + session_init_in->protocol = data->protocol; }
> > +EXPORT_SYMBOL(i915_hdcp_fill_session_in);
> > +
> > +void
> > +i915_hdcp_fill_rxcert_in(struct wired_cmd_verify_receiver_cert_in
> *verify_rxcert_in,
> > +struct hdcp2_ake_send_cert *rx_cert,
> > +struct hdcp_port_data *data)
> > +{
> > + verify_rxcert_in->header.api_version = HDCP_API_VERSION;
> > + verify_rxcert_in->header.command_id =
> WIRED_VERIFY_RECEIVER_CERT;
> > + verify_rxcert_in->header.status = FW_HDCP_STATUS_SUCCESS;
> > + verify_rxcert_in->header.buffer_len =
> > +
> WIRED_CMD_BUF_LEN_VERIFY_RECEIVER_CERT_IN;
> > +
> > + verify_rxcert_in->port.integrated_port_type = data->port_type;
> > + verify_rxcert_in->port.physical_port = (u8)data->hdcp_ddi;
> > + verify_rxcert_in->port.attached_transcoder =
> > +(u8)data->hdcp_transcoder;
> > +
> > + verify_rxcert_in->cert_rx = rx_cert->cert_rx;
> > + memcpy(verify_rxcert_in->r_rx, &rx_cert->r_rx, HDCP_2_2_RRX_LEN);
> > + memcpy(verify_rxcert_in->rx_caps, rx_cert->rx_caps,
> > +HDCP_2_2_RXCAPS_LEN); } EXPORT_SYMBOL(i915_hdcp_fill_rxcert_in);
> > +
> > +void
> > +i915_hdcp_fill_hprime_in(struct wired_cmd_ake_send_hprime_in
> *send_hprime_in,
> > +struct hdcp2_ake_send_hprime *rx_hprime,
> > +struct hdcp_port_data *data)
> > +{
> > + send_hprime_in->header.api_version = HDCP_API_VERSION;
> > + send_hprime_in->header.command_id = WIRED_AKE_SEND_HPRIME;
> > + send_hprime_in->header.status = FW_HDCP_STATUS_SUCCESS;
> > + send_hprime_in->header.buffer_len =
> > +WIRED_CMD_BUF_LEN_AKE_SEND_HPRIME_IN;
> > +
> > + send_hprime_in->port.integrated_port_type = data->port_type;
> > + send_hprime_in->port.physical_port = (u8)data->hdcp_ddi;
> > + send_hprime_in->port.attached_transcoder =
> > +(u8)data->hdcp_transcoder;
> > +
> > + memcpy(send_hprime_in->h_prime, rx_hprime->h_prime,
> > + HDCP_2_2_H_PRIME_LEN);
> > +}
> > +EXPORT_SYMBOL(i915_hdcp_fill_hprime_in);
> > +
> > +void
> > +i915_hdcp_fill_pairing_info_in(struct wired_cmd_ak
Re: [Intel-gfx] [PATCH v4 5/7] drm/i915/hdcp: Fill wired_cmd_in structures at a single place
Just my 2 cents (im not hdcp expert but understand a little on the GSC vs MEI):
IIRC, HW architecture wise, MEI and GSC-CS (command streamer) are both merely
transport protocols to a backend-service.
The backend-service here being HDCP-subsystem within the security firmware (one
of several backend-services within the security firmware).
The hdcp service, like the other services (PXP or others not in upstream), uses
a fw-API for comms with the backend-service.
This API is different based on the target backend-service (i.e. HDCP vs PXP vs
others). In all cases, MEI or GSC-CS are merely the transport layer.
That said, HW architecture wise, MEI and GSC-CS are like the
blitter-cmd-streamer - the transport, not the actual engine.
Citing the blitter example, the actual engine specific instructions with its
own "API" (i.e. HW micro-arch instructions such as XY_COLOR_BLT) need to be
crafted by the caller
or requesting client. In i915, we dont put functions in the engine code that
abstract the API crafting. We put API crafting in the caller side (even if its
a kernel space
caler).
Thus, adopting that blue-print, in this HDCP case, MEI-or-GSC-CS is the
transport and i915 display subsystem is the requesting client so i915 should be
crafting the
instructions.
However, access to the gsc-cs transport should not go thru a component
interface (did i misunderstand Jani's comment?) ... it should be contained i915
just like any other
engine today - i believe thats the hw arch direction forward (i.e. for all
firmware security services that are specific to the GPU should be via the
GSC-CS front end - that
has the same "HW layer positioning" as the other command-streamers).
...alan
On Tue, 2022-12-27 at 18:45 +0200, Jani Nikula wrote:
> On Thu, 22 Dec 2022, Suraj Kandpal wrote:
> > Need to fill wired cmd in structures at a single place as they remain
> > same for both gsc and mei.
>
> It's probably a good idea to have them in a single place, but there's
> really no reason for that place to be part of i915 at all.
>
> None of the functions are about i915.
>
> Feels weird to have MEI HDCP call back to i915. It's supposed to be the
> glue layer between i915 and ME FW. Maybe the abstractions are wrong?
>
> We currently have a total of 5 exported symbols, and I'd prefer to keep
> it that way.
>
>
> BR,
> Jani.
>
>
> >
> > --v3
> > -remove inline function from header [Jani]
> >
> > Cc: Ankit Nautiyal
> > Signed-off-by: Suraj Kandpal
> > ---
> > drivers/gpu/drm/i915/Makefile | 1 +
> > drivers/gpu/drm/i915/i915_hdcp_interface.c | 216 +
> > drivers/misc/mei/hdcp/mei_hdcp.c | 153 ++-
> > include/drm/i915_hdcp_interface.h | 39
> > 4 files changed, 270 insertions(+), 139 deletions(-)
> > create mode 100644 drivers/gpu/drm/i915/i915_hdcp_interface.c
> >
> > diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> > index 5b5df71cb5d7..b0b2a47455df 100644
> > --- a/drivers/gpu/drm/i915/Makefile
> > +++ b/drivers/gpu/drm/i915/Makefile
> > @@ -36,6 +36,7 @@ i915-y += i915_driver.o \
> > i915_drm_client.o \
> > i915_config.o \
> > i915_getparam.o \
> > + i915_hdcp_interface.o\
> > i915_ioctl.o \
> > i915_irq.o \
> > i915_mitigations.o \
> > diff --git a/drivers/gpu/drm/i915/i915_hdcp_interface.c
> > b/drivers/gpu/drm/i915/i915_hdcp_interface.c
> > new file mode 100644
> > index ..e6b787c2fa50
> > --- /dev/null
> > +++ b/drivers/gpu/drm/i915/i915_hdcp_interface.c
> > @@ -0,0 +1,216 @@
> > +// SPDX-License-Identifier: MIT
> > +/*
> > + * Copyright 2022, Intel Corporation.
> > + */
> > +
> > +#include
> > +
> > +void
> > +i915_hdcp_fill_session_in(struct wired_cmd_initiate_hdcp2_session_in
> > *session_init_in,
> > + struct hdcp_port_data *data)
> > +{
> > + session_init_in->header.api_version = HDCP_API_VERSION;
> > + session_init_in->header.command_id = WIRED_INITIATE_HDCP2_SESSION;
> > + session_init_in->header.status = FW_HDCP_STATUS_SUCCESS;
> > + session_init_in->header.buffer_len =
> > + WIRED_CMD_BUF_LEN_INITIATE_HDCP2_SESSION_IN;
> > +
> > + session_init_in->port.integrated_port_type = data->port_type;
> > + session_init_in->port.physical_port = (u8)data->hdcp_ddi;
> > + session_init_in->port.attached_transcoder = (u8)data->hdcp_transcoder;
> > + session_init_in->protocol = data->protocol;
> > +}
> > +EXPORT_SYMBOL(i915_hdcp_fill_session_in);
> > +
> > +void
> > +i915_hdcp_fill_rxcert_in(struct wired_cmd_verify_receiver_cert_in
> > *verify_rxcert_in,
> > +struct hdcp2_ake_send_cert *rx_cert,
> > +struct hdcp_port_data *data)
> > +{
> > + verify_rxcert_in->header.api_version = HDCP_API_VERSION;
> > + verify_rxcert_in->header.command_id = WIRED_VERIFY_RECEIVER_CERT;
> > + verify_rxcert_in->header.status = FW_HDCP_STATUS_SUCCESS;
> > + verify_rxcer
Re: [Intel-gfx] [PATCH v4 5/7] drm/i915/hdcp: Fill wired_cmd_in structures at a single place
On Thu, 22 Dec 2022, Suraj Kandpal wrote:
> Need to fill wired cmd in structures at a single place as they remain
> same for both gsc and mei.
It's probably a good idea to have them in a single place, but there's
really no reason for that place to be part of i915 at all.
None of the functions are about i915.
Feels weird to have MEI HDCP call back to i915. It's supposed to be the
glue layer between i915 and ME FW. Maybe the abstractions are wrong?
We currently have a total of 5 exported symbols, and I'd prefer to keep
it that way.
BR,
Jani.
>
> --v3
> -remove inline function from header [Jani]
>
> Cc: Ankit Nautiyal
> Signed-off-by: Suraj Kandpal
> ---
> drivers/gpu/drm/i915/Makefile | 1 +
> drivers/gpu/drm/i915/i915_hdcp_interface.c | 216 +
> drivers/misc/mei/hdcp/mei_hdcp.c | 153 ++-
> include/drm/i915_hdcp_interface.h | 39
> 4 files changed, 270 insertions(+), 139 deletions(-)
> create mode 100644 drivers/gpu/drm/i915/i915_hdcp_interface.c
>
> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> index 5b5df71cb5d7..b0b2a47455df 100644
> --- a/drivers/gpu/drm/i915/Makefile
> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -36,6 +36,7 @@ i915-y += i915_driver.o \
> i915_drm_client.o \
> i915_config.o \
> i915_getparam.o \
> + i915_hdcp_interface.o\
> i915_ioctl.o \
> i915_irq.o \
> i915_mitigations.o \
> diff --git a/drivers/gpu/drm/i915/i915_hdcp_interface.c
> b/drivers/gpu/drm/i915/i915_hdcp_interface.c
> new file mode 100644
> index ..e6b787c2fa50
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/i915_hdcp_interface.c
> @@ -0,0 +1,216 @@
> +// SPDX-License-Identifier: MIT
> +/*
> + * Copyright 2022, Intel Corporation.
> + */
> +
> +#include
> +
> +void
> +i915_hdcp_fill_session_in(struct wired_cmd_initiate_hdcp2_session_in
> *session_init_in,
> + struct hdcp_port_data *data)
> +{
> + session_init_in->header.api_version = HDCP_API_VERSION;
> + session_init_in->header.command_id = WIRED_INITIATE_HDCP2_SESSION;
> + session_init_in->header.status = FW_HDCP_STATUS_SUCCESS;
> + session_init_in->header.buffer_len =
> + WIRED_CMD_BUF_LEN_INITIATE_HDCP2_SESSION_IN;
> +
> + session_init_in->port.integrated_port_type = data->port_type;
> + session_init_in->port.physical_port = (u8)data->hdcp_ddi;
> + session_init_in->port.attached_transcoder = (u8)data->hdcp_transcoder;
> + session_init_in->protocol = data->protocol;
> +}
> +EXPORT_SYMBOL(i915_hdcp_fill_session_in);
> +
> +void
> +i915_hdcp_fill_rxcert_in(struct wired_cmd_verify_receiver_cert_in
> *verify_rxcert_in,
> + struct hdcp2_ake_send_cert *rx_cert,
> + struct hdcp_port_data *data)
> +{
> + verify_rxcert_in->header.api_version = HDCP_API_VERSION;
> + verify_rxcert_in->header.command_id = WIRED_VERIFY_RECEIVER_CERT;
> + verify_rxcert_in->header.status = FW_HDCP_STATUS_SUCCESS;
> + verify_rxcert_in->header.buffer_len =
> + WIRED_CMD_BUF_LEN_VERIFY_RECEIVER_CERT_IN;
> +
> + verify_rxcert_in->port.integrated_port_type = data->port_type;
> + verify_rxcert_in->port.physical_port = (u8)data->hdcp_ddi;
> + verify_rxcert_in->port.attached_transcoder = (u8)data->hdcp_transcoder;
> +
> + verify_rxcert_in->cert_rx = rx_cert->cert_rx;
> + memcpy(verify_rxcert_in->r_rx, &rx_cert->r_rx, HDCP_2_2_RRX_LEN);
> + memcpy(verify_rxcert_in->rx_caps, rx_cert->rx_caps,
> HDCP_2_2_RXCAPS_LEN);
> +}
> +EXPORT_SYMBOL(i915_hdcp_fill_rxcert_in);
> +
> +void
> +i915_hdcp_fill_hprime_in(struct wired_cmd_ake_send_hprime_in *send_hprime_in,
> + struct hdcp2_ake_send_hprime *rx_hprime,
> + struct hdcp_port_data *data)
> +{
> + send_hprime_in->header.api_version = HDCP_API_VERSION;
> + send_hprime_in->header.command_id = WIRED_AKE_SEND_HPRIME;
> + send_hprime_in->header.status = FW_HDCP_STATUS_SUCCESS;
> + send_hprime_in->header.buffer_len =
> WIRED_CMD_BUF_LEN_AKE_SEND_HPRIME_IN;
> +
> + send_hprime_in->port.integrated_port_type = data->port_type;
> + send_hprime_in->port.physical_port = (u8)data->hdcp_ddi;
> + send_hprime_in->port.attached_transcoder = (u8)data->hdcp_transcoder;
> +
> + memcpy(send_hprime_in->h_prime, rx_hprime->h_prime,
> +HDCP_2_2_H_PRIME_LEN);
> +}
> +EXPORT_SYMBOL(i915_hdcp_fill_hprime_in);
> +
> +void
> +i915_hdcp_fill_pairing_info_in(struct wired_cmd_ake_send_pairing_info_in
> *pairing_info_in,
> +struct hdcp2_ake_send_pairing_info *pairing_info,
> +struct hdcp_port_data *data)
> +{
> + pairing_info_in->header.api_version = HDCP_API_VERSION;
> + pairing_info_in->header.command_id = WIRED_AKE_SEND_PAIRING_INFO;
> + pairing_info_in->header.status = FW_HDCP_ST
