[Intel-gfx] [PATCH v2 10/42] misc/mei/hdcp: Initiate Wired HDCP2.2 Tx Session

[Winkler, Tomas]

> 
> 
> 
> On Thursday 08 March 2018 06:47 PM, Winkler, Tomas wrote:
> >> Request ME FW to start the HDCP2.2 session for a intel port.
> >> Prepares payloads for command WIRED_INITIATE_HDCP2_SESSION and
> sent
> >> to ME FW.
> >>
> >> On Success, ME FW will start a HDCP2.2 session for the port and
> >> provides the content for HDCP2.2 AKE_Init message.
> >>
> >> v2:
> >>    Rebased.
> >>
> >> Signed-off-by: Ramalingam C <ramalingam.c at intel.com>
> >> ---
> >>   drivers/misc/mei/hdcp/mei_hdcp.c | 73
> >> ++++++++++++++++++++++++++++++++++++++++
> >>   include/linux/mei_hdcp.h         | 11 ++++++
> >>   2 files changed, 84 insertions(+)
> >>
> >> diff --git a/drivers/misc/mei/hdcp/mei_hdcp.c
> >> b/drivers/misc/mei/hdcp/mei_hdcp.c
> >> index 63f77800a6f7..516ad6a40616 100644
> >> --- a/drivers/misc/mei/hdcp/mei_hdcp.c
> >> +++ b/drivers/misc/mei/hdcp/mei_hdcp.c
> >> @@ -29,6 +29,7 @@
> >>   #include <linux/module.h>
> >>   #include <linux/slab.h>
> >>   #include <linux/uuid.h>
> >> +#include <drm/drm_connector.h>
> >>
> >>   #include "mei_hdcp.h"
> >>
> >> @@ -46,6 +47,78 @@ static inline bool
> >> mei_cldev_active_and_enabled(struct mei_cl_device *cldev)
> >>   	return mei_cldev_enabled(cldev);
> >>   }
> >>
> >> +/**
> >> + * mei_initiate_hdcp2_session:
> >> + *	Function to start a Wired HDCP2.2 Tx Session with ME FW
> >> + *
> >> + * @data		: Intel HW specific Data
> >> + * @ake_data		: ptr to store AKE_Init
> >> + *
> >> + * Returns 0 on Success, <0 on Failure.
> >> + */
> >> +int mei_initiate_hdcp2_session(struct mei_hdcp_data *data,
> >> +			       struct hdcp2_ake_init *ake_data) {
> >
> > You should use cldev as a first argument for all those functions
> data has it as a member. Should it be explicit?


I believe, you should.
cldev is the object this function operates on so it should be explicitly and data provides the input paramterss

Thanks
Tomas 

> >
> >> +	/* check for the mei_device enabled or not */
> >> +	if (!mei_cldev_active_and_enabled(data->cldev))
> >> +		return -ENODEV;
> > No reason cldev will be NULL here.
> Ok. But device could be disabled right? we might want to check the enabled
> state of the device?

The device might go under reset any time during operations.
so it is useless to test at this point (unless you know some other flow to take)
as the underlying service already checks that for you and will return  appropriate error (ENODEV).
Thanks
Tomas

> >
> >> +	dev = &data->cldev->dev;
> >> +
> >> +	/* Fill header details */
> > Those types of comments are redundant.
> >> +	session_init_in.header.api_version = HDCP_API_VERSION;
> >> +	session_init_in.header.command_id =
> >> WIRED_INITIATE_HDCP2_SESSION;
> >> +	session_init_in.header.status = ME_HDCP_STATUS_SUCCESS;
> >> +	session_init_in.header.buffer_len =
> >> +
> >> 	WIRED_CMD_BUF_LEN_INITIATE_HDCP2_SESSION_IN;
> >> +
> >> +	/* Fill in the In Data */
> > Those types of comments are redundant.
> >> +	session_init_in.port.integrated_port_type = data->port_type;
> >> +	session_init_in.port.physical_port = data->port;
> >> +	session_init_in.protocol = (uint8_t)data->protocol;
> >> +
> >> +	/* Request to ME */
> >> +	byte = mei_cldev_send(data->cldev, (u8 *)&session_init_in,
> >> +			      sizeof(session_init_in));
> >> +	if (byte < 0) {
> >> +		dev_err(dev, "mei_cldev_send failed. %d\n", (int)byte);
> >> +		return byte;
> >> +	}
> >> +
> >> +	/* Response from ME */
> >> +	byte = mei_cldev_recv(data->cldev, (u8 *)&session_init_out,
> >> +			      sizeof(session_init_out));
> >> +	if (byte < 0) {
> >> +		dev_err(dev, "mei_cldev_recv failed. %d\n", (int)byte);
> >> +		return byte;
> >> +	}
> >> +
> >> +	status = (enum me_hdcp_status)session_init_out.header.status;
> > Useless cast
> Oops. Will remove it.
> 
> --Ram
> >> +	if (status != ME_HDCP_STATUS_SUCCESS) {
> >> +		dev_err(dev, "ME cmd 0x%08X Failed. Status: 0x%X\n",
> >> +			WIRED_INITIATE_HDCP2_SESSION, status);
> >> +		return -1;
> >> +	}
> >> +
> >> +	ake_data->msg_id = HDCP_2_2_AKE_INIT;
> >> +	ake_data->tx_caps = session_init_out.tx_caps;
> >> +	memcpy(ake_data->r_tx, session_init_out.r_tx,
> >> +	       sizeof(session_init_out.r_tx));
> >> +
> >> +	return 0;
> >> +}
> >> +EXPORT_SYMBOL(mei_initiate_hdcp2_session);
> >> +
> >>   static int mei_hdcp_probe(struct mei_cl_device *cldev,
> >>   			  const struct mei_cl_device_id *id)  { diff --git
> >> a/include/linux/mei_hdcp.h b/include/linux/mei_hdcp.h index
> >> 9a869d1cbd5d..c333528b9c1e 100644
> >> --- a/include/linux/mei_hdcp.h
> >> +++ b/include/linux/mei_hdcp.h
> >> @@ -24,6 +24,7 @@
> >>   #define _LINUX_MEI_HDCP_H
> >>
> >>   #include <linux/mei_cl_bus.h>
> >> +#include <drm/drm_hdcp.h>
> >>
> >>   /*
> >>    * Enumeration of the physical DDI available on the platform @@
> >> -101,6
> >> +102,9 @@ int mei_hdcp_cldev_get_reference(void *client_data,
> >>   						       struct mei_cl_device
> >>   						       *cldev));
> >>   void mei_hdcp_cldev_put_reference(struct mei_cl_device *cldev);
> >> +
> >> +int mei_initiate_hdcp2_session(struct mei_hdcp_data *data,
> >> +			       struct hdcp2_ake_init *ake_data);
> >>   #else
> >>   static inline
> >>   int mei_hdcp_cldev_get_reference(void *client_data, @@ -114,5
> >> +118,12 @@ int mei_hdcp_cldev_get_reference(void *client_data,
> >> static inline  void mei_hdcp_cldev_put_reference(struct mei_cl_device
> >> *cldev)  {}
> >> +
> >> +static inline
> >> +int mei_initiate_hdcp2_session(struct mei_hdcp_data *data,
> >> +			       struct hdcp2_ake_init *ake_data) {
> >> +	return -ENODEV;
> >> +}
> >>   #endif /* defined (CONFIG_INTEL_MEI_HDCP) */  #endif /* defined
> >> (_LINUX_MEI_HDCP_H) */
> >> --
> >> 2.7.4