[v8,03/35] linux/mei: Header for mei_hdcp driver interface

Submitted by Ramalingam C on Nov. 27, 2018, 10:43 a.m.

Details

Message ID 1543315413-24302-4-git-send-email-ramalingam.c@intel.com
State New
Headers show
Series "drm/i915: Implement HDCP2.2" ( rev: 10 ) in Intel GFX

Not browsing as part of any series.

Commit Message

Ramalingam C Nov. 27, 2018, 10:43 a.m.
Data structures and Enum for the I915-MEI_HDCP interface are defined
at <linux/mei_hdcp.h>

v2:
  Rebased.
v3:
  mei_cl_device is removed from mei_hdcp_data [Tomas]
v4:
  Comment style and typo fixed [Uma]
v5:
  Rebased.
v6:
  No changes.
v7:
  Remove redundant text from the License header
  Change uintXX_t type to uXX_t types
  Remove unneeded include to mei_cl_bus.h
  Coding style fixed [Uma]
V8:
  Tab cleanup
  Fix kdoc and namespaces
  Update MAINTAINERS

Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
Reviewed-by: Uma Shankar <uma.shankar@intel.com>
---
 MAINTAINERS              |  1 +
 include/linux/mei_hdcp.h | 91 ++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 92 insertions(+)
 create mode 100644 include/linux/mei_hdcp.h

Patch hide | download patch | download mbox

diff --git a/MAINTAINERS b/MAINTAINERS
index 1026150ae90f..2fd6555bf040 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -7540,6 +7540,7 @@  L:	linux-kernel@vger.kernel.org
 S:	Supported
 F:	include/uapi/linux/mei.h
 F:	include/linux/mei_cl_bus.h
+F:	include/linux/mei_hdcp.h
 F:	drivers/misc/mei/*
 F:	drivers/watchdog/mei_wdt.c
 F:	Documentation/misc-devices/mei/*
diff --git a/include/linux/mei_hdcp.h b/include/linux/mei_hdcp.h
new file mode 100644
index 000000000000..716123003dd1
--- /dev/null
+++ b/include/linux/mei_hdcp.h
@@ -0,0 +1,91 @@ 
+/* SPDX-License-Identifier: (GPL-2.0+ OR BSD-3-Clause) */
+/*
+ * Copyright © 2017-2018 Intel Corporation
+ *
+ * Authors:
+ * Ramalingam C <ramalingam.c@intel.com>
+ */
+
+#ifndef _LINUX_MEI_HDCP_H
+#define _LINUX_MEI_HDCP_H
+
+/**
+ * enum mei_hdcp_ddi - The physical digital display interface (DDI)
+ *     available on the platform
+ * @MEI_DDI_INVALID_PORT: Not a valid port
+ * @MEI_DDI_RANGE_BEGIN: Beginning of the valid DDI port range
+ * @MEI_DDI_B: Port DDI B
+ * @MEI_DDI_C: Port DDI C
+ * @MEI_DDI_D: Port DDI D
+ * @MEI_DDI_E: Port DDI E
+ * @MEI_DDI_F: Port DDI F
+ * @MEI_DDI_A: Port DDI A
+ * @MEI_DDI_RANGE_END: End of the valid DDI port range
+ */
+enum mei_hdcp_ddi {
+	MEI_DDI_INVALID_PORT = 0x00,
+
+	MEI_DDI_RANGE_BEGIN = 0x01,
+	MEI_DDI_B           = 0x01,
+	MEI_DDI_C           = 0x02,
+	MEI_DDI_D           = 0x03,
+	MEI_DDI_E           = 0x04,
+	MEI_DDI_F           = 0x05,
+	MEI_DDI_A           = 0x07,
+	MEI_DDI_RANGE_END   = MEI_DDI_A,
+};
+
+/**
+ * enum mei_hdcp_port_type - The types of HDCP 2.2 ports supported
+ *
+ * @MEI_HDCP_PORT_TYPE_INVALID: Invalid port
+ * @MEI_HDCP_PORT_TYPE_INTEGRATED: ports that are integrated into Intel HW
+ * @MEI_HDCP_PORT_TYPE_LSPCON: discrete wired Tx port with LSPCON (HDMI 2.0)
+ * @MEI_HDCP_PORT_TYPE_CPDP: discrete wired Tx port using the CPDP (DP 1.3)
+ */
+enum mei_hdcp_port_type {
+	MEI_HDCP_PORT_TYPE_INVALID    = 0x00,
+	MEI_HDCP_PORT_TYPE_INTEGRATED = 0x01,
+	MEI_HDCP_PORT_TYPE_LSPCON     = 0x02,
+	MEI_HDCP_PORT_TYPE_CPDP       = 0x03,
+};
+
+/*
+ * enum mei_hdcp_wired_protocol - Supported integrated wired HDCP protocol.
+ * @HDCP_PROTOCOL_INVALID: invalid type
+ * @HDCP_PROTOCOL_HDMI: HDMI
+ * @HDCP_PROTOCOL_DP: DP
+ *
+ * Based on this value, Minor difference needed between wired specifications
+ * are handled.
+ */
+enum mei_hdcp_wired_protocol {
+	MEI_HDCP_PROTOCOL_INVALID,
+	MEI_HDCP_PROTOCOL_HDMI,
+	MEI_HDCP_PROTOCOL_DP
+};
+
+/**
+ * struct mei_hdcp_data - Input data to the mei_hdcp APIs
+ * @port: The physical port (ddi).
+ * @port_type: The port type.
+ * @protocol: The Protocol on the port.
+ * @k: Number of streams transmitted on the port.
+ *     In case of HDMI & DP SST, a single stream will be
+ *     transmitted on the port.
+ * @seq_num_m: A sequence number of RepeaterAuth_Stream_Manage msg propagated.
+ *     Initialized to 0 on AKE_INIT. Incremented after every successful
+ *     transmission of RepeaterAuth_Stream_Manage message. When it rolls
+ *     over re-Auth has to be triggered.
+ * @streams: array[k] of streamid
+ */
+struct mei_hdcp_data {
+	enum mei_hdcp_ddi port;
+	enum mei_hdcp_port_type port_type;
+	enum mei_hdcp_wired_protocol protocol;
+	u16 k;
+	u32 seq_num_m;
+	struct hdcp2_streamid_type *streams;
+};
+
+#endif /* !_LINUX_MEI_HDCP_H */

Comments

Hi,

In one of the offline discussion Tomas has shared his review comments on v8.
So I am sharing the abstract of his suggestions here for the discussion and for the agreement of interface in the community.
Tomas please correct/add if I am missing any points.

 1. Remove the include/linux/mei_hdcp.h to make the i915-mei interface
    more generic.
     1. Move the definition of the struct mei_hdcp_data to i915 and
        mei_hdcp.c and pass the void* in the ops' functions.
     2. Move the conversion of enum port value to mei_ddi_port value
        into mei_hdcp.c. Let I915 pass the enum port value as such.
     3. Modified local definition of the struct mei_hdcp_data will looks
        like
     4.

        +/* hdcp data per port */
        +struct hdcp_port_data {
        + short int port;
        + u8 port_type;
        + u8 protocol;
        + u16 k;
        + u32 seq_num_m;
        + struct hdcp2_streamid_type *streams;
          };

 2. Add K-Doc compliant commenting in the mei_hdcp.c

I have implemented these changes and posted for intel-gfx-trybot. Just incase anyone wants to
refer the code please look at https://patchwork.freedesktop.org/series/53655/ .
Not shared on #intel-gfx as further review discussions are on-going on intel-gfx.

--Ram

On 11/27/2018 4:13 PM, Ramalingam C wrote:
> Data structures and Enum for the I915-MEI_HDCP interface are defined
> at <linux/mei_hdcp.h>
>
> v2:
>    Rebased.
> v3:
>    mei_cl_device is removed from mei_hdcp_data [Tomas]
> v4:
>    Comment style and typo fixed [Uma]
> v5:
>    Rebased.
> v6:
>    No changes.
> v7:
>    Remove redundant text from the License header
>    Change uintXX_t type to uXX_t types
>    Remove unneeded include to mei_cl_bus.h
>    Coding style fixed [Uma]
> V8:
>    Tab cleanup
>    Fix kdoc and namespaces
>    Update MAINTAINERS
>
> Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
> Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
> Reviewed-by: Uma Shankar <uma.shankar@intel.com>
> ---
>   MAINTAINERS              |  1 +
>   include/linux/mei_hdcp.h | 91 ++++++++++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 92 insertions(+)
>   create mode 100644 include/linux/mei_hdcp.h
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 1026150ae90f..2fd6555bf040 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -7540,6 +7540,7 @@ L:	linux-kernel@vger.kernel.org
>   S:	Supported
>   F:	include/uapi/linux/mei.h
>   F:	include/linux/mei_cl_bus.h
> +F:	include/linux/mei_hdcp.h
>   F:	drivers/misc/mei/*
>   F:	drivers/watchdog/mei_wdt.c
>   F:	Documentation/misc-devices/mei/*
> diff --git a/include/linux/mei_hdcp.h b/include/linux/mei_hdcp.h
> new file mode 100644
> index 000000000000..716123003dd1
> --- /dev/null
> +++ b/include/linux/mei_hdcp.h
> @@ -0,0 +1,91 @@
> +/* SPDX-License-Identifier: (GPL-2.0+ OR BSD-3-Clause) */
> +/*
> + * Copyright © 2017-2018 Intel Corporation
> + *
> + * Authors:
> + * Ramalingam C <ramalingam.c@intel.com>
> + */
> +
> +#ifndef _LINUX_MEI_HDCP_H
> +#define _LINUX_MEI_HDCP_H
> +
> +/**
> + * enum mei_hdcp_ddi - The physical digital display interface (DDI)
> + *     available on the platform
> + * @MEI_DDI_INVALID_PORT: Not a valid port
> + * @MEI_DDI_RANGE_BEGIN: Beginning of the valid DDI port range
> + * @MEI_DDI_B: Port DDI B
> + * @MEI_DDI_C: Port DDI C
> + * @MEI_DDI_D: Port DDI D
> + * @MEI_DDI_E: Port DDI E
> + * @MEI_DDI_F: Port DDI F
> + * @MEI_DDI_A: Port DDI A
> + * @MEI_DDI_RANGE_END: End of the valid DDI port range
> + */
> +enum mei_hdcp_ddi {
> +	MEI_DDI_INVALID_PORT = 0x00,
> +
> +	MEI_DDI_RANGE_BEGIN = 0x01,
> +	MEI_DDI_B           = 0x01,
> +	MEI_DDI_C           = 0x02,
> +	MEI_DDI_D           = 0x03,
> +	MEI_DDI_E           = 0x04,
> +	MEI_DDI_F           = 0x05,
> +	MEI_DDI_A           = 0x07,
> +	MEI_DDI_RANGE_END   = MEI_DDI_A,
> +};
> +
> +/**
> + * enum mei_hdcp_port_type - The types of HDCP 2.2 ports supported
> + *
> + * @MEI_HDCP_PORT_TYPE_INVALID: Invalid port
> + * @MEI_HDCP_PORT_TYPE_INTEGRATED: ports that are integrated into Intel HW
> + * @MEI_HDCP_PORT_TYPE_LSPCON: discrete wired Tx port with LSPCON (HDMI 2.0)
> + * @MEI_HDCP_PORT_TYPE_CPDP: discrete wired Tx port using the CPDP (DP 1.3)
> + */
> +enum mei_hdcp_port_type {
> +	MEI_HDCP_PORT_TYPE_INVALID    = 0x00,
> +	MEI_HDCP_PORT_TYPE_INTEGRATED = 0x01,
> +	MEI_HDCP_PORT_TYPE_LSPCON     = 0x02,
> +	MEI_HDCP_PORT_TYPE_CPDP       = 0x03,
> +};
> +
> +/*
> + * enum mei_hdcp_wired_protocol - Supported integrated wired HDCP protocol.
> + * @HDCP_PROTOCOL_INVALID: invalid type
> + * @HDCP_PROTOCOL_HDMI: HDMI
> + * @HDCP_PROTOCOL_DP: DP
> + *
> + * Based on this value, Minor difference needed between wired specifications
> + * are handled.
> + */
> +enum mei_hdcp_wired_protocol {
> +	MEI_HDCP_PROTOCOL_INVALID,
> +	MEI_HDCP_PROTOCOL_HDMI,
> +	MEI_HDCP_PROTOCOL_DP
> +};
> +
> +/**
> + * struct mei_hdcp_data - Input data to the mei_hdcp APIs
> + * @port: The physical port (ddi).
> + * @port_type: The port type.
> + * @protocol: The Protocol on the port.
> + * @k: Number of streams transmitted on the port.
> + *     In case of HDMI & DP SST, a single stream will be
> + *     transmitted on the port.
> + * @seq_num_m: A sequence number of RepeaterAuth_Stream_Manage msg propagated.
> + *     Initialized to 0 on AKE_INIT. Incremented after every successful
> + *     transmission of RepeaterAuth_Stream_Manage message. When it rolls
> + *     over re-Auth has to be triggered.
> + * @streams: array[k] of streamid
> + */
> +struct mei_hdcp_data {
> +	enum mei_hdcp_ddi port;
> +	enum mei_hdcp_port_type port_type;
> +	enum mei_hdcp_wired_protocol protocol;
> +	u16 k;
> +	u32 seq_num_m;
> +	struct hdcp2_streamid_type *streams;
> +};
> +
> +#endif /* !_LINUX_MEI_HDCP_H */
On Fri, Dec 07, 2018 at 07:23:06PM +0530, C, Ramalingam wrote:
> Hi,
> 
> In one of the offline discussion Tomas has shared his review comments on v8.

Let's please have all review here on the mailing list for better
coordination. Playing a game of telephone isn't efficient.

> So I am sharing the abstract of his suggestions here for the discussion and for the agreement of interface in the community.
> Tomas please correct/add if I am missing any points.
> 
> 1. Remove the include/linux/mei_hdcp.h to make the i915-mei interface
>    more generic.
>     1. Move the definition of the struct mei_hdcp_data to i915 and
>        mei_hdcp.c and pass the void* in the ops' functions.

I don't get this. Using void * instead of the actual type we're passing
isn't more generic, it's just less safe. If we later on need to extend the
api contract between mei_hdcp and i915 we can always do that. Like we
already do with the i915/snd-hda-intel component contract in
i915_component.h and drm_audio_component.h.

Aside: Header names for the audio interface are maybe not the best, this
isn't primarily a component thing. So maybe call it
i915_mei_hdcp_interface.h and stuff all the structures/function pointers
that define the interface between the two drivers in there. Or some other
suitable name you like better.

>     2. Move the conversion of enum port value to mei_ddi_port value
>        into mei_hdcp.c. Let I915 pass the enum port value as such.

logical port 2 physical register index mapping tends to shift around and
is always highly machine specific. As long as we do it consistently
somewhere we should be good. Seems fine to me.

>     3. Modified local definition of the struct mei_hdcp_data will looks
>        like

No local defintions of structures please. Otherwise I'm ok with whatever
gets the job done.

>     4.
> 
>        +/* hdcp data per port */
>        +struct hdcp_port_data {
>        + short int port;
>        + u8 port_type;
>        + u8 protocol;
>        + u16 k;
>        + u32 seq_num_m;
>        + struct hdcp2_streamid_type *streams;
>          };
> 
> 2. Add K-Doc compliant commenting in the mei_hdcp.c

If you do that, please include the relevant comments into the drm/i915
docbook, like we do already with the audio stuff.

> I have implemented these changes and posted for intel-gfx-trybot. Just incase anyone wants to
> refer the code please look at https://patchwork.freedesktop.org/series/53655/ .
> Not shared on #intel-gfx as further review discussions are on-going on intel-gfx.

As discussed, no void * in the interface, and we definitely need a shared
header for ops/data structures. We want the compiler to help us catch when
one side of this i915/mei_hdcp api contract changes as much as possible.
All the other changes seem reasonable.

Thanks, Daniel

> 

> --Ram
> 
> On 11/27/2018 4:13 PM, Ramalingam C wrote:
> > Data structures and Enum for the I915-MEI_HDCP interface are defined
> > at <linux/mei_hdcp.h>
> > 
> > v2:
> >    Rebased.
> > v3:
> >    mei_cl_device is removed from mei_hdcp_data [Tomas]
> > v4:
> >    Comment style and typo fixed [Uma]
> > v5:
> >    Rebased.
> > v6:
> >    No changes.
> > v7:
> >    Remove redundant text from the License header
> >    Change uintXX_t type to uXX_t types
> >    Remove unneeded include to mei_cl_bus.h
> >    Coding style fixed [Uma]
> > V8:
> >    Tab cleanup
> >    Fix kdoc and namespaces
> >    Update MAINTAINERS
> > 
> > Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
> > Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
> > Reviewed-by: Uma Shankar <uma.shankar@intel.com>
> > ---
> >   MAINTAINERS              |  1 +
> >   include/linux/mei_hdcp.h | 91 ++++++++++++++++++++++++++++++++++++++++++++++++
> >   2 files changed, 92 insertions(+)
> >   create mode 100644 include/linux/mei_hdcp.h
> > 
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 1026150ae90f..2fd6555bf040 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -7540,6 +7540,7 @@ L:	linux-kernel@vger.kernel.org
> >   S:	Supported
> >   F:	include/uapi/linux/mei.h
> >   F:	include/linux/mei_cl_bus.h
> > +F:	include/linux/mei_hdcp.h
> >   F:	drivers/misc/mei/*
> >   F:	drivers/watchdog/mei_wdt.c
> >   F:	Documentation/misc-devices/mei/*
> > diff --git a/include/linux/mei_hdcp.h b/include/linux/mei_hdcp.h
> > new file mode 100644
> > index 000000000000..716123003dd1
> > --- /dev/null
> > +++ b/include/linux/mei_hdcp.h
> > @@ -0,0 +1,91 @@
> > +/* SPDX-License-Identifier: (GPL-2.0+ OR BSD-3-Clause) */
> > +/*
> > + * Copyright © 2017-2018 Intel Corporation
> > + *
> > + * Authors:
> > + * Ramalingam C <ramalingam.c@intel.com>
> > + */
> > +
> > +#ifndef _LINUX_MEI_HDCP_H
> > +#define _LINUX_MEI_HDCP_H
> > +
> > +/**
> > + * enum mei_hdcp_ddi - The physical digital display interface (DDI)
> > + *     available on the platform
> > + * @MEI_DDI_INVALID_PORT: Not a valid port
> > + * @MEI_DDI_RANGE_BEGIN: Beginning of the valid DDI port range
> > + * @MEI_DDI_B: Port DDI B
> > + * @MEI_DDI_C: Port DDI C
> > + * @MEI_DDI_D: Port DDI D
> > + * @MEI_DDI_E: Port DDI E
> > + * @MEI_DDI_F: Port DDI F
> > + * @MEI_DDI_A: Port DDI A
> > + * @MEI_DDI_RANGE_END: End of the valid DDI port range
> > + */
> > +enum mei_hdcp_ddi {
> > +	MEI_DDI_INVALID_PORT = 0x00,
> > +
> > +	MEI_DDI_RANGE_BEGIN = 0x01,
> > +	MEI_DDI_B           = 0x01,
> > +	MEI_DDI_C           = 0x02,
> > +	MEI_DDI_D           = 0x03,
> > +	MEI_DDI_E           = 0x04,
> > +	MEI_DDI_F           = 0x05,
> > +	MEI_DDI_A           = 0x07,
> > +	MEI_DDI_RANGE_END   = MEI_DDI_A,
> > +};
> > +
> > +/**
> > + * enum mei_hdcp_port_type - The types of HDCP 2.2 ports supported
> > + *
> > + * @MEI_HDCP_PORT_TYPE_INVALID: Invalid port
> > + * @MEI_HDCP_PORT_TYPE_INTEGRATED: ports that are integrated into Intel HW
> > + * @MEI_HDCP_PORT_TYPE_LSPCON: discrete wired Tx port with LSPCON (HDMI 2.0)
> > + * @MEI_HDCP_PORT_TYPE_CPDP: discrete wired Tx port using the CPDP (DP 1.3)
> > + */
> > +enum mei_hdcp_port_type {
> > +	MEI_HDCP_PORT_TYPE_INVALID    = 0x00,
> > +	MEI_HDCP_PORT_TYPE_INTEGRATED = 0x01,
> > +	MEI_HDCP_PORT_TYPE_LSPCON     = 0x02,
> > +	MEI_HDCP_PORT_TYPE_CPDP       = 0x03,
> > +};
> > +
> > +/*
> > + * enum mei_hdcp_wired_protocol - Supported integrated wired HDCP protocol.
> > + * @HDCP_PROTOCOL_INVALID: invalid type
> > + * @HDCP_PROTOCOL_HDMI: HDMI
> > + * @HDCP_PROTOCOL_DP: DP
> > + *
> > + * Based on this value, Minor difference needed between wired specifications
> > + * are handled.
> > + */
> > +enum mei_hdcp_wired_protocol {
> > +	MEI_HDCP_PROTOCOL_INVALID,
> > +	MEI_HDCP_PROTOCOL_HDMI,
> > +	MEI_HDCP_PROTOCOL_DP
> > +};
> > +
> > +/**
> > + * struct mei_hdcp_data - Input data to the mei_hdcp APIs
> > + * @port: The physical port (ddi).
> > + * @port_type: The port type.
> > + * @protocol: The Protocol on the port.
> > + * @k: Number of streams transmitted on the port.
> > + *     In case of HDMI & DP SST, a single stream will be
> > + *     transmitted on the port.
> > + * @seq_num_m: A sequence number of RepeaterAuth_Stream_Manage msg propagated.
> > + *     Initialized to 0 on AKE_INIT. Incremented after every successful
> > + *     transmission of RepeaterAuth_Stream_Manage message. When it rolls
> > + *     over re-Auth has to be triggered.
> > + * @streams: array[k] of streamid
> > + */
> > +struct mei_hdcp_data {
> > +	enum mei_hdcp_ddi port;
> > +	enum mei_hdcp_port_type port_type;
> > +	enum mei_hdcp_wired_protocol protocol;
> > +	u16 k;
> > +	u32 seq_num_m;
> > +	struct hdcp2_streamid_type *streams;
> > +};
> > +
> > +#endif /* !_LINUX_MEI_HDCP_H */
> On Fri, Dec 07, 2018 at 07:23:06PM +0530, C, Ramalingam wrote:
> > Hi,
> >
> > In one of the offline discussion Tomas has shared his review comments on v8.
> 
> Let's please have all review here on the mailing list for better coordination.
> Playing a game of telephone isn't efficient.
It's not different from IRC or meeting on a conference, after all we end up with code we can comment on. 
> 
> > So I am sharing the abstract of his suggestions here for the discussion and for
> the agreement of interface in the community.
> > Tomas please correct/add if I am missing any points.
> >
> > 1. Remove the include/linux/mei_hdcp.h to make the i915-mei interface
> >    more generic.
> >     1. Move the definition of the struct mei_hdcp_data to i915 and
> >        mei_hdcp.c and pass the void* in the ops' functions.
> 
> I don't get this. Using void * instead of the actual type we're passing isn't more
> generic, it's just less safe. If we later on need to extend the api contract
> between mei_hdcp and i915 we can always do that. Like we already do with the
> i915/snd-hda-intel component contract in i915_component.h and
> drm_audio_component.h.

No I haven't suggesting using void *, what I've suggest is to use HDCP construct instead of mei specific types.

> Aside: Header names for the audio interface are maybe not the best, this isn't
> primarily a component thing. So maybe call it i915_mei_hdcp_interface.h and
> stuff all the structures/function pointers that define the interface between the
> two drivers in there. Or some other suitable name you like better.
> 
> >     2. Move the conversion of enum port value to mei_ddi_port value
> >        into mei_hdcp.c. Let I915 pass the enum port value as such.
> 
> logical port 2 physical register index mapping tends to shift around and is
> always highly machine specific. As long as we do it consistently somewhere we
> should be good. Seems fine to me.
> 
> >     3. Modified local definition of the struct mei_hdcp_data will looks
> >        like
> 
> No local defintions of structures please. Otherwise I'm ok with whatever gets
> the job done.
> 
> >     4.
> >
> >        +/* hdcp data per port */
> >        +struct hdcp_port_data {
> >        + short int port;
> >        + u8 port_type;
> >        + u8 protocol;
> >        + u16 k;
> >        + u32 seq_num_m;
> >        + struct hdcp2_streamid_type *streams;
> >          };
> >
> > 2. Add K-Doc compliant commenting in the mei_hdcp.c
> 
> If you do that, please include the relevant comments into the drm/i915
> docbook, like we do already with the audio stuff.
> 
> > I have implemented these changes and posted for intel-gfx-trybot. Just
> > incase anyone wants to refer the code please look at
> https://patchwork.freedesktop.org/series/53655/ .
> > Not shared on #intel-gfx as further review discussions are on-going on intel-
> gfx.
> 
> As discussed, no void * in the interface, and we definitely need a shared header
> for ops/data structures. We want the compiler to help us catch when one side
> of this i915/mei_hdcp api contract changes as much as possible.
> All the other changes seem reasonable.

> Thanks, Daniel
I agree with no void *, that was not my intention. 
It will be better to comment on v9 series. 

 
> >
> 
> > --Ram
> >
> > On 11/27/2018 4:13 PM, Ramalingam C wrote:
> > > Data structures and Enum for the I915-MEI_HDCP interface are defined
> > > at <linux/mei_hdcp.h>
> > >
> > > v2:
> > >    Rebased.
> > > v3:
> > >    mei_cl_device is removed from mei_hdcp_data [Tomas]
> > > v4:
> > >    Comment style and typo fixed [Uma]
> > > v5:
> > >    Rebased.
> > > v6:
> > >    No changes.
> > > v7:
> > >    Remove redundant text from the License header
> > >    Change uintXX_t type to uXX_t types
> > >    Remove unneeded include to mei_cl_bus.h
> > >    Coding style fixed [Uma]
> > > V8:
> > >    Tab cleanup
> > >    Fix kdoc and namespaces
> > >    Update MAINTAINERS
> > >
> > > Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
> > > Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
> > > Reviewed-by: Uma Shankar <uma.shankar@intel.com>
> > > ---
> > >   MAINTAINERS              |  1 +
> > >   include/linux/mei_hdcp.h | 91
> ++++++++++++++++++++++++++++++++++++++++++++++++
> > >   2 files changed, 92 insertions(+)
> > >   create mode 100644 include/linux/mei_hdcp.h
> > >
> > > diff --git a/MAINTAINERS b/MAINTAINERS index
> > > 1026150ae90f..2fd6555bf040 100644
> > > --- a/MAINTAINERS
> > > +++ b/MAINTAINERS
> > > @@ -7540,6 +7540,7 @@ L:	linux-kernel@vger.kernel.org
> > >   S:	Supported
> > >   F:	include/uapi/linux/mei.h
> > >   F:	include/linux/mei_cl_bus.h
> > > +F:	include/linux/mei_hdcp.h
> > >   F:	drivers/misc/mei/*
> > >   F:	drivers/watchdog/mei_wdt.c
> > >   F:	Documentation/misc-devices/mei/*
> > > diff --git a/include/linux/mei_hdcp.h b/include/linux/mei_hdcp.h new
> > > file mode 100644 index 000000000000..716123003dd1
> > > --- /dev/null
> > > +++ b/include/linux/mei_hdcp.h
> > > @@ -0,0 +1,91 @@
> > > +/* SPDX-License-Identifier: (GPL-2.0+ OR BSD-3-Clause) */
> > > +/*
> > > + * Copyright (c) 2017-2018 Intel Corporation
> > > + *
> > > + * Authors:
> > > + * Ramalingam C <ramalingam.c@intel.com>  */
> > > +
> > > +#ifndef _LINUX_MEI_HDCP_H
> > > +#define _LINUX_MEI_HDCP_H
> > > +
> > > +/**
> > > + * enum mei_hdcp_ddi - The physical digital display interface (DDI)
> > > + *     available on the platform
> > > + * @MEI_DDI_INVALID_PORT: Not a valid port
> > > + * @MEI_DDI_RANGE_BEGIN: Beginning of the valid DDI port range
> > > + * @MEI_DDI_B: Port DDI B
> > > + * @MEI_DDI_C: Port DDI C
> > > + * @MEI_DDI_D: Port DDI D
> > > + * @MEI_DDI_E: Port DDI E
> > > + * @MEI_DDI_F: Port DDI F
> > > + * @MEI_DDI_A: Port DDI A
> > > + * @MEI_DDI_RANGE_END: End of the valid DDI port range  */ enum
> > > +mei_hdcp_ddi {
> > > +	MEI_DDI_INVALID_PORT = 0x00,
> > > +
> > > +	MEI_DDI_RANGE_BEGIN = 0x01,
> > > +	MEI_DDI_B           = 0x01,
> > > +	MEI_DDI_C           = 0x02,
> > > +	MEI_DDI_D           = 0x03,
> > > +	MEI_DDI_E           = 0x04,
> > > +	MEI_DDI_F           = 0x05,
> > > +	MEI_DDI_A           = 0x07,
> > > +	MEI_DDI_RANGE_END   = MEI_DDI_A,
> > > +};
> > > +
> > > +/**
> > > + * enum mei_hdcp_port_type - The types of HDCP 2.2 ports supported
> > > + *
> > > + * @MEI_HDCP_PORT_TYPE_INVALID: Invalid port
> > > + * @MEI_HDCP_PORT_TYPE_INTEGRATED: ports that are integrated into
> > > +Intel HW
> > > + * @MEI_HDCP_PORT_TYPE_LSPCON: discrete wired Tx port with LSPCON
> > > +(HDMI 2.0)
> > > + * @MEI_HDCP_PORT_TYPE_CPDP: discrete wired Tx port using the CPDP
> > > +(DP 1.3)  */ enum mei_hdcp_port_type {
> > > +	MEI_HDCP_PORT_TYPE_INVALID    = 0x00,
> > > +	MEI_HDCP_PORT_TYPE_INTEGRATED = 0x01,
> > > +	MEI_HDCP_PORT_TYPE_LSPCON     = 0x02,
> > > +	MEI_HDCP_PORT_TYPE_CPDP       = 0x03,
> > > +};
> > > +
> > > +/*
> > > + * enum mei_hdcp_wired_protocol - Supported integrated wired HDCP
> protocol.
> > > + * @HDCP_PROTOCOL_INVALID: invalid type
> > > + * @HDCP_PROTOCOL_HDMI: HDMI
> > > + * @HDCP_PROTOCOL_DP: DP
> > > + *
> > > + * Based on this value, Minor difference needed between wired
> > > +specifications
> > > + * are handled.
> > > + */
> > > +enum mei_hdcp_wired_protocol {
> > > +	MEI_HDCP_PROTOCOL_INVALID,
> > > +	MEI_HDCP_PROTOCOL_HDMI,
> > > +	MEI_HDCP_PROTOCOL_DP
> > > +};
> > > +
> > > +/**
> > > + * struct mei_hdcp_data - Input data to the mei_hdcp APIs
> > > + * @port: The physical port (ddi).
> > > + * @port_type: The port type.
> > > + * @protocol: The Protocol on the port.
> > > + * @k: Number of streams transmitted on the port.
> > > + *     In case of HDMI & DP SST, a single stream will be
> > > + *     transmitted on the port.
> > > + * @seq_num_m: A sequence number of RepeaterAuth_Stream_Manage
> msg propagated.
> > > + *     Initialized to 0 on AKE_INIT. Incremented after every successful
> > > + *     transmission of RepeaterAuth_Stream_Manage message. When it
> rolls
> > > + *     over re-Auth has to be triggered.
> > > + * @streams: array[k] of streamid
> > > + */
> > > +struct mei_hdcp_data {
> > > +	enum mei_hdcp_ddi port;
> > > +	enum mei_hdcp_port_type port_type;
> > > +	enum mei_hdcp_wired_protocol protocol;
> > > +	u16 k;
> > > +	u32 seq_num_m;
> > > +	struct hdcp2_streamid_type *streams; };
> > > +
> > > +#endif /* !_LINUX_MEI_HDCP_H */
> 
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
On Sat, Dec 08, 2018 at 08:15:52PM +0000, Winkler, Tomas wrote:
> 
> > On Fri, Dec 07, 2018 at 07:23:06PM +0530, C, Ramalingam wrote:
> > > Hi,
> > >
> > > In one of the offline discussion Tomas has shared his review comments on v8.
> > 
> > Let's please have all review here on the mailing list for better coordination.
> > Playing a game of telephone isn't efficient.
> It's not different from IRC or meeting on a conference, after all we end up with code we can comment on. 

For IRC we try to summarize discussions on the mailing list too. Same for
conferences (if that ever happens for public stuff).

> > > So I am sharing the abstract of his suggestions here for the discussion and for
> > the agreement of interface in the community.
> > > Tomas please correct/add if I am missing any points.
> > >
> > > 1. Remove the include/linux/mei_hdcp.h to make the i915-mei interface
> > >    more generic.
> > >     1. Move the definition of the struct mei_hdcp_data to i915 and
> > >        mei_hdcp.c and pass the void* in the ops' functions.
> > 
> > I don't get this. Using void * instead of the actual type we're passing isn't more
> > generic, it's just less safe. If we later on need to extend the api contract
> > between mei_hdcp and i915 we can always do that. Like we already do with the
> > i915/snd-hda-intel component contract in i915_component.h and
> > drm_audio_component.h.
> 
> No I haven't suggesting using void *, what I've suggest is to use HDCP construct instead of mei specific types.

Ah, makes sense. I thought v7 pretty much does that already though, at
least as far as we define these structures on the drm side (it's just a
register file in an i2c target address after all).
-Daniel

> > Aside: Header names for the audio interface are maybe not the best, this isn't
> > primarily a component thing. So maybe call it i915_mei_hdcp_interface.h and
> > stuff all the structures/function pointers that define the interface between the
> > two drivers in there. Or some other suitable name you like better.
> > 
> > >     2. Move the conversion of enum port value to mei_ddi_port value
> > >        into mei_hdcp.c. Let I915 pass the enum port value as such.
> > 
> > logical port 2 physical register index mapping tends to shift around and is
> > always highly machine specific. As long as we do it consistently somewhere we
> > should be good. Seems fine to me.
> > 
> > >     3. Modified local definition of the struct mei_hdcp_data will looks
> > >        like
> > 
> > No local defintions of structures please. Otherwise I'm ok with whatever gets
> > the job done.
> > 
> > >     4.
> > >
> > >        +/* hdcp data per port */
> > >        +struct hdcp_port_data {
> > >        + short int port;
> > >        + u8 port_type;
> > >        + u8 protocol;
> > >        + u16 k;
> > >        + u32 seq_num_m;
> > >        + struct hdcp2_streamid_type *streams;
> > >          };
> > >
> > > 2. Add K-Doc compliant commenting in the mei_hdcp.c
> > 
> > If you do that, please include the relevant comments into the drm/i915
> > docbook, like we do already with the audio stuff.
> > 
> > > I have implemented these changes and posted for intel-gfx-trybot. Just
> > > incase anyone wants to refer the code please look at
> > https://patchwork.freedesktop.org/series/53655/ .
> > > Not shared on #intel-gfx as further review discussions are on-going on intel-
> > gfx.
> > 
> > As discussed, no void * in the interface, and we definitely need a shared header
> > for ops/data structures. We want the compiler to help us catch when one side
> > of this i915/mei_hdcp api contract changes as much as possible.
> > All the other changes seem reasonable.
> 
> > Thanks, Daniel
> I agree with no void *, that was not my intention. 
> It will be better to comment on v9 series. 
> 
>  
> > >
> > 
> > > --Ram
> > >
> > > On 11/27/2018 4:13 PM, Ramalingam C wrote:
> > > > Data structures and Enum for the I915-MEI_HDCP interface are defined
> > > > at <linux/mei_hdcp.h>
> > > >
> > > > v2:
> > > >    Rebased.
> > > > v3:
> > > >    mei_cl_device is removed from mei_hdcp_data [Tomas]
> > > > v4:
> > > >    Comment style and typo fixed [Uma]
> > > > v5:
> > > >    Rebased.
> > > > v6:
> > > >    No changes.
> > > > v7:
> > > >    Remove redundant text from the License header
> > > >    Change uintXX_t type to uXX_t types
> > > >    Remove unneeded include to mei_cl_bus.h
> > > >    Coding style fixed [Uma]
> > > > V8:
> > > >    Tab cleanup
> > > >    Fix kdoc and namespaces
> > > >    Update MAINTAINERS
> > > >
> > > > Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
> > > > Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
> > > > Reviewed-by: Uma Shankar <uma.shankar@intel.com>
> > > > ---
> > > >   MAINTAINERS              |  1 +
> > > >   include/linux/mei_hdcp.h | 91
> > ++++++++++++++++++++++++++++++++++++++++++++++++
> > > >   2 files changed, 92 insertions(+)
> > > >   create mode 100644 include/linux/mei_hdcp.h
> > > >
> > > > diff --git a/MAINTAINERS b/MAINTAINERS index
> > > > 1026150ae90f..2fd6555bf040 100644
> > > > --- a/MAINTAINERS
> > > > +++ b/MAINTAINERS
> > > > @@ -7540,6 +7540,7 @@ L:	linux-kernel@vger.kernel.org
> > > >   S:	Supported
> > > >   F:	include/uapi/linux/mei.h
> > > >   F:	include/linux/mei_cl_bus.h
> > > > +F:	include/linux/mei_hdcp.h
> > > >   F:	drivers/misc/mei/*
> > > >   F:	drivers/watchdog/mei_wdt.c
> > > >   F:	Documentation/misc-devices/mei/*
> > > > diff --git a/include/linux/mei_hdcp.h b/include/linux/mei_hdcp.h new
> > > > file mode 100644 index 000000000000..716123003dd1
> > > > --- /dev/null
> > > > +++ b/include/linux/mei_hdcp.h
> > > > @@ -0,0 +1,91 @@
> > > > +/* SPDX-License-Identifier: (GPL-2.0+ OR BSD-3-Clause) */
> > > > +/*
> > > > + * Copyright (c) 2017-2018 Intel Corporation
> > > > + *
> > > > + * Authors:
> > > > + * Ramalingam C <ramalingam.c@intel.com>  */
> > > > +
> > > > +#ifndef _LINUX_MEI_HDCP_H
> > > > +#define _LINUX_MEI_HDCP_H
> > > > +
> > > > +/**
> > > > + * enum mei_hdcp_ddi - The physical digital display interface (DDI)
> > > > + *     available on the platform
> > > > + * @MEI_DDI_INVALID_PORT: Not a valid port
> > > > + * @MEI_DDI_RANGE_BEGIN: Beginning of the valid DDI port range
> > > > + * @MEI_DDI_B: Port DDI B
> > > > + * @MEI_DDI_C: Port DDI C
> > > > + * @MEI_DDI_D: Port DDI D
> > > > + * @MEI_DDI_E: Port DDI E
> > > > + * @MEI_DDI_F: Port DDI F
> > > > + * @MEI_DDI_A: Port DDI A
> > > > + * @MEI_DDI_RANGE_END: End of the valid DDI port range  */ enum
> > > > +mei_hdcp_ddi {
> > > > +	MEI_DDI_INVALID_PORT = 0x00,
> > > > +
> > > > +	MEI_DDI_RANGE_BEGIN = 0x01,
> > > > +	MEI_DDI_B           = 0x01,
> > > > +	MEI_DDI_C           = 0x02,
> > > > +	MEI_DDI_D           = 0x03,
> > > > +	MEI_DDI_E           = 0x04,
> > > > +	MEI_DDI_F           = 0x05,
> > > > +	MEI_DDI_A           = 0x07,
> > > > +	MEI_DDI_RANGE_END   = MEI_DDI_A,
> > > > +};
> > > > +
> > > > +/**
> > > > + * enum mei_hdcp_port_type - The types of HDCP 2.2 ports supported
> > > > + *
> > > > + * @MEI_HDCP_PORT_TYPE_INVALID: Invalid port
> > > > + * @MEI_HDCP_PORT_TYPE_INTEGRATED: ports that are integrated into
> > > > +Intel HW
> > > > + * @MEI_HDCP_PORT_TYPE_LSPCON: discrete wired Tx port with LSPCON
> > > > +(HDMI 2.0)
> > > > + * @MEI_HDCP_PORT_TYPE_CPDP: discrete wired Tx port using the CPDP
> > > > +(DP 1.3)  */ enum mei_hdcp_port_type {
> > > > +	MEI_HDCP_PORT_TYPE_INVALID    = 0x00,
> > > > +	MEI_HDCP_PORT_TYPE_INTEGRATED = 0x01,
> > > > +	MEI_HDCP_PORT_TYPE_LSPCON     = 0x02,
> > > > +	MEI_HDCP_PORT_TYPE_CPDP       = 0x03,
> > > > +};
> > > > +
> > > > +/*
> > > > + * enum mei_hdcp_wired_protocol - Supported integrated wired HDCP
> > protocol.
> > > > + * @HDCP_PROTOCOL_INVALID: invalid type
> > > > + * @HDCP_PROTOCOL_HDMI: HDMI
> > > > + * @HDCP_PROTOCOL_DP: DP
> > > > + *
> > > > + * Based on this value, Minor difference needed between wired
> > > > +specifications
> > > > + * are handled.
> > > > + */
> > > > +enum mei_hdcp_wired_protocol {
> > > > +	MEI_HDCP_PROTOCOL_INVALID,
> > > > +	MEI_HDCP_PROTOCOL_HDMI,
> > > > +	MEI_HDCP_PROTOCOL_DP
> > > > +};
> > > > +
> > > > +/**
> > > > + * struct mei_hdcp_data - Input data to the mei_hdcp APIs
> > > > + * @port: The physical port (ddi).
> > > > + * @port_type: The port type.
> > > > + * @protocol: The Protocol on the port.
> > > > + * @k: Number of streams transmitted on the port.
> > > > + *     In case of HDMI & DP SST, a single stream will be
> > > > + *     transmitted on the port.
> > > > + * @seq_num_m: A sequence number of RepeaterAuth_Stream_Manage
> > msg propagated.
> > > > + *     Initialized to 0 on AKE_INIT. Incremented after every successful
> > > > + *     transmission of RepeaterAuth_Stream_Manage message. When it
> > rolls
> > > > + *     over re-Auth has to be triggered.
> > > > + * @streams: array[k] of streamid
> > > > + */
> > > > +struct mei_hdcp_data {
> > > > +	enum mei_hdcp_ddi port;
> > > > +	enum mei_hdcp_port_type port_type;
> > > > +	enum mei_hdcp_wired_protocol protocol;
> > > > +	u16 k;
> > > > +	u32 seq_num_m;
> > > > +	struct hdcp2_streamid_type *streams; };
> > > > +
> > > > +#endif /* !_LINUX_MEI_HDCP_H */
> > 
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch