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

From: Daniel Vetter <daniel@ffwll.ch>
To: "C, Ramalingam" <ramalingam.c@intel.com>
Cc: daniel.vetter@ffwll.ch, intel-gfx@lists.freedesktop.org,
	tomas.winkler@intel.com, dri-devel@lists.freedesktop.org
Subject: Re: [PATCH v8 03/35] linux/mei: Header for mei_hdcp driver interface
Date: Fri, 7 Dec 2018 15:10:29 +0100	[thread overview]
Message-ID: <20181207141029.GB21184@phenom.ffwll.local> (raw)
In-Reply-To: <1bde7d59-db1f-a7cf-9333-d903cafd8e42@intel.com>

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 */

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx