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@xxxxxxxxx>
> > > > Signed-off-by: Tomas Winkler <tomas.winkler@xxxxxxxxx>
> > > > Reviewed-by: Uma Shankar <uma.shankar@xxxxxxxxx>
> > > > ---
> > > > 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@xxxxxxxxxxxxxxx
> > > > 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@xxxxxxxxx> */
> > > > +
> > > > +#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
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
