Re: [PATCH v5 01/40] drm: hdcp2.2 authentication msg definitions

[Sean Paul <seanpaul@chromium.org>]

On Wed, Jul 11, 2018 at 05:57:08PM +0000, C, Ramalingam wrote:
> Thanks seanpaul for the reviews.
> 
> > -----Original Message-----
> > From: Sean Paul [mailto:seanpaul@chromium.org]
> > Sent: Tuesday, July 10, 2018 1:51 AM
> > To: C, Ramalingam <ramalingam.c@intel.com>
> > Cc: intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org;
> > daniel@ffwll.ch; Winkler, Tomas <tomas.winkler@intel.com>; Usyskin,
> > Alexander <alexander.usyskin@intel.com>; Shankar, Uma
> > <uma.shankar@intel.com>
> > Subject: Re: [Intel-gfx] [PATCH v5 01/40] drm: hdcp2.2 authentication msg
> > definitions
> > 
> > On Wed, Jun 27, 2018 at 02:09:50PM +0530, Ramalingam C wrote:
> > > This patch defines the hdcp2.2 protocol messages for authentication.
> > >
> > > v2:
> > >   bit_fields are removed. Instead bitmasking used. [Tomas and Jani]
> > >   prefix HDCP_2_2_ is added to the macros. [Tomas]
> > > v3:
> > >   No Changes.
> > > v4:
> > >   Style and spellings are fixed [Uma]
> > > v5:
> > >   Fix for macros.
> > >
> > > Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
> > > ---
> > >  include/drm/drm_hdcp.h | 179
> > > +++++++++++++++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 179 insertions(+)
> > >
> > > diff --git a/include/drm/drm_hdcp.h b/include/drm/drm_hdcp.h index
> > > 98e63d870139..3e963c5d04b2 100644
> > > --- a/include/drm/drm_hdcp.h
> > > +++ b/include/drm/drm_hdcp.h
> > > @@ -38,4 +38,183 @@
> > >  #define DRM_HDCP_DDC_BSTATUS			0x41
> > >  #define DRM_HDCP_DDC_KSV_FIFO			0x43
> > >
> > > +#define DRM_HDCP_1_4_SRM_ID			0x8
> > > +#define DRM_HDCP_1_4_VRL_LENGTH_SIZE		3
> > > +#define DRM_HDCP_1_4_DCP_SIG_SIZE		40
> > 
> > These don't seem to be related to the patch?
> > 
> > > +
> > > +/* Protocol message definition for HDCP2.2 specification */
> > > +#define HDCP_STREAM_TYPE0			0x00
> > > +#define HDCP_STREAM_TYPE1			0x01
> > 
> > Why not HDCP_2_2 prefix?
> Though they are introduced at HDCP2.2, this is classification of the streams.
> And Type 0 can be transmitted on HDCP1.4.
> So keeping it as generic name with no version mentioned.

Ok, I guess it's the comment that was throwing me off. Perhaps you could improve
it to:

/*
 * Protected video streams are classified into 2 types:
 * - Type0: Can be transmitted with HDCP 1.4+
 * - Type1: Can be transmitted with HDCP 2.2+
 */

/snip

> > > +} __packed;
> > 
> > Perhaps this has already been asked and answered, but do all of these need to
> > be __packed? This is kind of the problem with adding a bunch of unused
> > structures to a patch, it's hard to see what their usage is. In future, these should
> > probably be introduced when they're being used.
> 
> These are the HDCP2.2 message defined at HDCP2.2 spec. And they needs to be
> __packed just to have exact size mentioned by spec.
> 
> Like how we have HDCP1.4 and 2.2 macros defined as per the HDCP spec definitions,
> defined the HDCP2.2 messages together here.

Thanks for the explanation.

Sean

> 
> Thanks,
> Ram.
> > 
> > Sean
> > 
> > > +
> > >  #endif
> > > --
> > > 2.7.4
> > >
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 
> > --
> > Sean Paul, Software Engineer, Google / Chromium OS

-- 
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx