Re: [Intel-gfx] [PATCH v8 05/35] drm/i915: MEI interface definition

2018-12-12 Thread C, Ramalingam


On 12/12/2018 4:34 PM, C, Ramalingam wrote:



On 12/12/2018 4:08 PM, Daniel Vetter wrote:

On Wed, Dec 12, 2018 at 02:28:29PM +0530, C, Ramalingam wrote:

On 12/7/2018 7:59 PM, Daniel Vetter wrote:

On Fri, Dec 07, 2018 at 11:22:44AM +0530, C, Ramalingam wrote:

On 12/6/2018 3:53 PM, Daniel Vetter wrote:

On Tue, Nov 27, 2018 at 04:13:03PM +0530, Ramalingam C wrote:

+static void i915_hdcp_component_master_unbind(struct device *dev)
+{
+   struct drm_i915_private *dev_priv = kdev_to_i915(dev);
+
+   intel_connectors_hdcp_disable(dev_priv);

Why this code? Once we've unregistered the driver, we should have shut
down the hardware completely. Which should have shut down all the hdcp
users too.

This unbind might be triggered either due to master_del or component_del.
if its triggered from mei through component_del, then before teardown of
the i/f in component_unbind(), disable the ongoing HDCP session through
hdcp_disable() for each connectors.

I looked at your v7 component code again. I think if we put the
drm_atomic_helper_shutdown() call into master_unbind, then we'll have taken care
of that. Essentially what you're doing here is open-coding part of that
function. Better to just move the existing call to the right place.

And shutting down the entire hw is kinda what master_unbind should be
doing I think. We might also need to move the general hw quiescent into
master_unbind, that should work too.

Need some more information on this. As per v7 on master_unbind will invoke
i915_unload_head that is i915_driver_unregister(dev_priv);
Similarly master_bind will call the load_tail that is 
i915_driver_register(dev_priv);

We are not initializing/shutting the HW in master_bind/unbind .
But this comment is contradicting with current approach. Could you please 
elaborate.?

So my understanding is that we need to shut down all hdcp usage before
master_unbind completes, because otherwise we'll blow up: The mei_hdcp
component is gone already.

Now the other case for master_unbind is that the i915 pci device is
unloaded, and in that case again I think it makes sense to shut down the
hardware in master_unbind.

Now when I looked at v7, right after the component_unbind code in the i915
unload sequences comes the function calls to shut down the hardware. I
think it would make sense to them (or at least the
drm_atomic_helper_shutdown() call) into master_unbind.

This is somewhat asymetric, but that's ok: Load path doesn't enable
anything, we just keep the hardware as-is (to be able to support
flicker-free boôt-up). First modest is done by usersapce. Exception is if
you have fbcon enabled (and not use the fastboot patches that Hans just
merged), in that case the kernel will do the first modeset when we
regiseter the fbdev device.

Anyway, moving the drm_atomic_helper_shutdown() into the master_unbind
function in v7 should take care of disabling all outputs, and hence also
disabling all usage of hdcp component.

But in that case master_bind should do the reverse of the 
drm_atomic_helper_shutdown()right?
else lets say mei_hdcp is removed, master_unbind triggered and hence all hw is 
shutdown.
And then mei_hdcp is loaded so master_bind should initialize the hw right? 
Which is not the scenario right now.


Summarizing the #intel-gfx IRC discussion:

As i915 load and unload  are already asymetric, there is no harm in moving
the drm_atomic_helper_shutdown() into the master_unbind(). So going
ahead with daniel's suggestion.

-Ram



-Ram

-Daniel
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx


Re: [Intel-gfx] [PATCH v8 05/35] drm/i915: MEI interface definition

2018-12-12 Thread C, Ramalingam


On 12/12/2018 4:08 PM, Daniel Vetter wrote:

On Wed, Dec 12, 2018 at 02:28:29PM +0530, C, Ramalingam wrote:

On 12/7/2018 7:59 PM, Daniel Vetter wrote:

On Fri, Dec 07, 2018 at 11:22:44AM +0530, C, Ramalingam wrote:

On 12/6/2018 3:53 PM, Daniel Vetter wrote:

On Tue, Nov 27, 2018 at 04:13:03PM +0530, Ramalingam C wrote:

+static void i915_hdcp_component_master_unbind(struct device *dev)
+{
+   struct drm_i915_private *dev_priv = kdev_to_i915(dev);
+
+   intel_connectors_hdcp_disable(dev_priv);

Why this code? Once we've unregistered the driver, we should have shut
down the hardware completely. Which should have shut down all the hdcp
users too.

This unbind might be triggered either due to master_del or component_del.
if its triggered from mei through component_del, then before teardown of
the i/f in component_unbind(), disable the ongoing HDCP session through
hdcp_disable() for each connectors.

I looked at your v7 component code again. I think if we put the
drm_atomic_helper_shutdown() call into master_unbind, then we'll have taken care
of that. Essentially what you're doing here is open-coding part of that
function. Better to just move the existing call to the right place.

And shutting down the entire hw is kinda what master_unbind should be
doing I think. We might also need to move the general hw quiescent into
master_unbind, that should work too.

Need some more information on this. As per v7 on master_unbind will invoke
i915_unload_head that is i915_driver_unregister(dev_priv);
Similarly master_bind will call the load_tail that is 
i915_driver_register(dev_priv);

We are not initializing/shutting the HW in master_bind/unbind .
But this comment is contradicting with current approach. Could you please 
elaborate.?

So my understanding is that we need to shut down all hdcp usage before
master_unbind completes, because otherwise we'll blow up: The mei_hdcp
component is gone already.

Now the other case for master_unbind is that the i915 pci device is
unloaded, and in that case again I think it makes sense to shut down the
hardware in master_unbind.

Now when I looked at v7, right after the component_unbind code in the i915
unload sequences comes the function calls to shut down the hardware. I
think it would make sense to them (or at least the
drm_atomic_helper_shutdown() call) into master_unbind.

This is somewhat asymetric, but that's ok: Load path doesn't enable
anything, we just keep the hardware as-is (to be able to support
flicker-free boôt-up). First modest is done by usersapce. Exception is if
you have fbcon enabled (and not use the fastboot patches that Hans just
merged), in that case the kernel will do the first modeset when we
regiseter the fbdev device.

Anyway, moving the drm_atomic_helper_shutdown() into the master_unbind
function in v7 should take care of disabling all outputs, and hence also
disabling all usage of hdcp component.


But in that case master_bind should do the reverse of the 
drm_atomic_helper_shutdown()right?
else lets say mei_hdcp is removed, master_unbind triggered and hence all hw is 
shutdown.
And then mei_hdcp is loaded so master_bind should initialize the hw right? 
Which is not the scenario right now.

-Ram


-Daniel
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx


Re: [Intel-gfx] [PATCH v8 05/35] drm/i915: MEI interface definition

2018-12-12 Thread Daniel Vetter
On Wed, Dec 12, 2018 at 02:28:29PM +0530, C, Ramalingam wrote:
> On 12/7/2018 7:59 PM, Daniel Vetter wrote:
> > On Fri, Dec 07, 2018 at 11:22:44AM +0530, C, Ramalingam wrote:
> > > On 12/6/2018 3:53 PM, Daniel Vetter wrote:
> > > > On Tue, Nov 27, 2018 at 04:13:03PM +0530, Ramalingam C wrote:
> > > > > +static void i915_hdcp_component_master_unbind(struct device *dev)
> > > > > +{
> > > > > + struct drm_i915_private *dev_priv = kdev_to_i915(dev);
> > > > > +
> > > > > + intel_connectors_hdcp_disable(dev_priv);
> > > > Why this code? Once we've unregistered the driver, we should have shut
> > > > down the hardware completely. Which should have shut down all the hdcp
> > > > users too.
> > > This unbind might be triggered either due to master_del or component_del.
> > > if its triggered from mei through component_del, then before teardown of
> > > the i/f in component_unbind(), disable the ongoing HDCP session through
> > > hdcp_disable() for each connectors.
> > I looked at your v7 component code again. I think if we put the
> > drm_atomic_helper_shutdown() call into master_unbind, then we'll have taken 
> > care
> > of that. Essentially what you're doing here is open-coding part of that
> > function. Better to just move the existing call to the right place.
> > 
> > And shutting down the entire hw is kinda what master_unbind should be
> > doing I think. We might also need to move the general hw quiescent into
> > master_unbind, that should work too.
> 
> Need some more information on this. As per v7 on master_unbind will invoke
> i915_unload_head that is i915_driver_unregister(dev_priv);
> Similarly master_bind will call the load_tail that is 
> i915_driver_register(dev_priv);
> 
> We are not initializing/shutting the HW in master_bind/unbind .
> But this comment is contradicting with current approach. Could you please 
> elaborate.?

So my understanding is that we need to shut down all hdcp usage before
master_unbind completes, because otherwise we'll blow up: The mei_hdcp
component is gone already.

Now the other case for master_unbind is that the i915 pci device is
unloaded, and in that case again I think it makes sense to shut down the
hardware in master_unbind.

Now when I looked at v7, right after the component_unbind code in the i915
unload sequences comes the function calls to shut down the hardware. I
think it would make sense to them (or at least the
drm_atomic_helper_shutdown() call) into master_unbind.

This is somewhat asymetric, but that's ok: Load path doesn't enable
anything, we just keep the hardware as-is (to be able to support
flicker-free boôt-up). First modest is done by usersapce. Exception is if
you have fbcon enabled (and not use the fastboot patches that Hans just
merged), in that case the kernel will do the first modeset when we
regiseter the fbdev device.

Anyway, moving the drm_atomic_helper_shutdown() into the master_unbind
function in v7 should take care of disabling all outputs, and hence also
disabling all usage of hdcp component.
-Daniel
-- 
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


Re: [Intel-gfx] [PATCH v8 05/35] drm/i915: MEI interface definition

2018-12-12 Thread C, Ramalingam


On 12/7/2018 7:59 PM, Daniel Vetter wrote:

On Fri, Dec 07, 2018 at 11:22:44AM +0530, C, Ramalingam wrote:

On 12/6/2018 3:53 PM, Daniel Vetter wrote:

On Tue, Nov 27, 2018 at 04:13:03PM +0530, Ramalingam C wrote:

Defining the mei-i915 interface functions and initialization of
the interface.

Signed-off-by: Ramalingam C 
Signed-off-by: Tomas Winkler 
---
   drivers/gpu/drm/i915/i915_drv.h   |   2 +
   drivers/gpu/drm/i915/intel_drv.h  |   7 +
   drivers/gpu/drm/i915/intel_hdcp.c | 442 
+-
   include/drm/i915_component.h  |  71 ++
   4 files changed, 521 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index f763b30f98d9..b68bc980b7cd 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2015,6 +2015,8 @@ struct drm_i915_private {
struct i915_pmu pmu;
+   struct i915_hdcp_component_master *hdcp_comp;
+
/*
 * NOTE: This is the dri1/ums dungeon, don't add stuff here. Your patch
 * will be rejected. Instead look for a better place.
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 85a526598096..bde82f3ada85 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -29,6 +29,7 @@
   #include 
   #include 
   #include 
+#include 
   #include 
   #include "i915_drv.h"
   #include 
@@ -379,6 +380,9 @@ struct intel_hdcp_shim {
/* Detects panel's hdcp capability. This is optional for HDMI. */
int (*hdcp_capable)(struct intel_digital_port *intel_dig_port,
bool *hdcp_capable);
+
+   /* Detects the HDCP protocol(DP/HDMI) required on the port */
+   enum mei_hdcp_wired_protocol (*hdcp_protocol)(void);

Looking ahead, this seems hardwired to constant return value? Or why do we
need a function here?

This is hardwired based on the connector type(DP/HDMI).
Since we have the shim for hdcp's connector based work, I have added this 
function.

Could have done this just with connector_type check, but in that way whole 
hdcp_shim
can be done in that way. So going with the larger design here.

If it's hardwired then just make it a hardwired struct member. As long as
it's all const, we can mix data an function pointers. If you have runtime
variable data, then it's better to split it out from the ops structure, so
that we can keep ops read-only and protected against possible exploits
(any function pointers are a high-value target in the kernel).

Sure. Done.

   };
   struct intel_hdcp {
@@ -399,6 +403,9 @@ struct intel_hdcp {
 * content can flow only through a link protected by HDCP2.2.
 */
u8 content_type;
+
+   /* mei interface related information */
+   struct mei_hdcp_data mei_data;
   };
   struct intel_connector {
diff --git a/drivers/gpu/drm/i915/intel_hdcp.c 
b/drivers/gpu/drm/i915/intel_hdcp.c
index 99dddb540958..760780f1105c 100644
--- a/drivers/gpu/drm/i915/intel_hdcp.c
+++ b/drivers/gpu/drm/i915/intel_hdcp.c
@@ -8,14 +8,20 @@
   #include 
   #include 
+#include 
   #include 
   #include 
+#include 
   #include "intel_drv.h"
   #include "i915_reg.h"
   #define KEY_LOAD_TRIES   5
   #define TIME_FOR_ENCRYPT_STATUS_CHANGE   50
+#define GET_MEI_DDI_INDEX(p) ({\
+   typeof(p) __p = (p);   \
+   __p == PORT_A ? MEI_DDI_A : (enum mei_hdcp_ddi)__p;\
+})
   static
   bool intel_hdcp_is_ksv_valid(u8 *ksv)
@@ -833,6 +839,417 @@ bool is_hdcp_supported(struct drm_i915_private *dev_priv, 
enum port port)
!IS_CHERRYVIEW(dev_priv) && port < PORT_E);
   }
+static __attribute__((unused)) int
+hdcp2_prepare_ake_init(struct intel_connector *connector,
+  struct hdcp2_ake_init *ake_data)
+{
+   struct mei_hdcp_data *data = >hdcp.mei_data;
+   struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
+   struct i915_hdcp_component_master *comp = dev_priv->hdcp_comp;
+   int ret;
+
+   if (!comp)
+   return -EINVAL;
+
+   mutex_lock(>mutex);
+   if (!comp->ops || !comp->dev) {
+   mutex_unlock(>mutex);
+   return -EINVAL;
+   }
+
+   if (data->port == MEI_DDI_INVALID_PORT && connector->encoder)
+   data->port = GET_MEI_DDI_INDEX(connector->encoder->port);
+
+   /* Clear ME FW instance for the port, just incase */
+   comp->ops->close_hdcp_session(comp->dev, data);

Sounds like a bug somewhere if we need this? I'd put this code into the
->initiate_hdcp2_session, with a big WARN_ON if it's actually needed.

Not really. Lets say you have progressed beyond the first step of HDCP2.2 auth 
along with ME FW.
Now if authentication failed due to some reason, then the HDCP2.2 season 
created with
ME FW for that port is not closed yet.

So we need to call close_hdcp_session() explicitly on authentication failures.
Session has 

Re: [Intel-gfx] [PATCH v8 05/35] drm/i915: MEI interface definition

2018-12-07 Thread Daniel Vetter
On Fri, Dec 07, 2018 at 04:18:25PM +0530, C, Ramalingam wrote:
> 
> On 12/7/2018 11:22 AM, C, Ramalingam wrote:
> > 
> > 
> > On 12/6/2018 3:53 PM, Daniel Vetter wrote:
> > > On Tue, Nov 27, 2018 at 04:13:03PM +0530, Ramalingam C wrote:
> > > > Defining the mei-i915 interface functions and initialization of
> > > > the interface.
> > > > 
> > > > Signed-off-by: Ramalingam C
> > > > Signed-off-by: Tomas Winkler
> > > > ---
> > > >   drivers/gpu/drm/i915/i915_drv.h   |   2 +
> > > >   drivers/gpu/drm/i915/intel_drv.h  |   7 +
> > > >   drivers/gpu/drm/i915/intel_hdcp.c | 442 
> > > > +-
> > > >   include/drm/i915_component.h  |  71 ++
> > > >   4 files changed, 521 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h 
> > > > b/drivers/gpu/drm/i915/i915_drv.h
> > > > index f763b30f98d9..b68bc980b7cd 100644
> > > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > > @@ -2015,6 +2015,8 @@ struct drm_i915_private {
> > > > struct i915_pmu pmu;
> > > > +   struct i915_hdcp_component_master *hdcp_comp;
> > > > +
> > > > /*
> > > >  * NOTE: This is the dri1/ums dungeon, don't add stuff here. 
> > > > Your patch
> > > >  * will be rejected. Instead look for a better place.
> > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h 
> > > > b/drivers/gpu/drm/i915/intel_drv.h
> > > > index 85a526598096..bde82f3ada85 100644
> > > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > > @@ -29,6 +29,7 @@
> > > >   #include 
> > > >   #include 
> > > >   #include 
> > > > +#include 
> > > >   #include 
> > > >   #include "i915_drv.h"
> > > >   #include 
> > > > @@ -379,6 +380,9 @@ struct intel_hdcp_shim {
> > > > /* Detects panel's hdcp capability. This is optional for HDMI. 
> > > > */
> > > > int (*hdcp_capable)(struct intel_digital_port *intel_dig_port,
> > > > bool *hdcp_capable);
> > > > +
> > > > +   /* Detects the HDCP protocol(DP/HDMI) required on the port */
> > > > +   enum mei_hdcp_wired_protocol (*hdcp_protocol)(void);
> > > Looking ahead, this seems hardwired to constant return value? Or why do we
> > > need a function here?
> > This is hardwired based on the connector type(DP/HDMI).
> > Since we have the shim for hdcp's connector based work, I have added this 
> > function.
> > 
> > Could have done this just with connector_type check, but in that way whole 
> > hdcp_shim
> > can be done in that way. So going with the larger design here.
> > > >   };
> > > >   struct intel_hdcp {
> > > > @@ -399,6 +403,9 @@ struct intel_hdcp {
> > > >  * content can flow only through a link protected by HDCP2.2.
> > > >  */
> > > > u8 content_type;
> > > > +
> > > > +   /* mei interface related information */
> > > > +   struct mei_hdcp_data mei_data;
> > > >   };
> > > >   struct intel_connector {
> > > > diff --git a/drivers/gpu/drm/i915/intel_hdcp.c 
> > > > b/drivers/gpu/drm/i915/intel_hdcp.c
> > > > index 99dddb540958..760780f1105c 100644
> > > > --- a/drivers/gpu/drm/i915/intel_hdcp.c
> > > > +++ b/drivers/gpu/drm/i915/intel_hdcp.c
> > > > @@ -8,14 +8,20 @@
> > > >   #include 
> > > >   #include 
> > > > +#include 
> > > >   #include 
> > > >   #include 
> > > > +#include 
> > > >   #include "intel_drv.h"
> > > >   #include "i915_reg.h"
> > > >   #define KEY_LOAD_TRIES5
> > > >   #define TIME_FOR_ENCRYPT_STATUS_CHANGE50
> > > > +#define GET_MEI_DDI_INDEX(p) ({\
> > > > +   typeof(p) __p = (p);   \
> > > > +   __p == PORT_A ? MEI_DDI_A : (enum mei_hdcp_ddi)__p;\
> > > > +})
> > > >   static
> > > >   bool intel_hdcp_is_ksv_valid(u8 *ksv)
> > > > @@ -833,6 +839,417 @@ bool is_hdcp_supported(struct drm_i915_private 
> > > > *dev_priv, enum port port)
> > > > !IS_CHERRYVIEW(dev_priv) && port < PORT_E);
> > > >   }
> > > > +static __attribute__((unused)) int
> > > > +hdcp2_prepare_ake_init(struct intel_connector *connector,
> > > > +  struct hdcp2_ake_init *ake_data)
> > > > +{
> > > > +   struct mei_hdcp_data *data = >hdcp.mei_data;
> > > > +   struct drm_i915_private *dev_priv = 
> > > > to_i915(connector->base.dev);
> > > > +   struct i915_hdcp_component_master *comp = dev_priv->hdcp_comp;
> > > > +   int ret;
> > > > +
> > > > +   if (!comp)
> > > > +   return -EINVAL;
> > > > +
> > > > +   mutex_lock(>mutex);
> > > > +   if (!comp->ops || !comp->dev) {
> > > > +   mutex_unlock(>mutex);
> > > > +   return -EINVAL;
> > > > +   }
> > > > +
> > > > +   if (data->port == MEI_DDI_INVALID_PORT && connector->encoder)
> > > > +   data->port = 
> > > > GET_MEI_DDI_INDEX(connector->encoder->port);
> > > > +
> > > > +   /* Clear ME FW instance for the port, 

Re: [Intel-gfx] [PATCH v8 05/35] drm/i915: MEI interface definition

2018-12-07 Thread Daniel Vetter
On Fri, Dec 07, 2018 at 11:22:44AM +0530, C, Ramalingam wrote:
> 
> On 12/6/2018 3:53 PM, Daniel Vetter wrote:
> > On Tue, Nov 27, 2018 at 04:13:03PM +0530, Ramalingam C wrote:
> > > Defining the mei-i915 interface functions and initialization of
> > > the interface.
> > > 
> > > Signed-off-by: Ramalingam C 
> > > Signed-off-by: Tomas Winkler 
> > > ---
> > >   drivers/gpu/drm/i915/i915_drv.h   |   2 +
> > >   drivers/gpu/drm/i915/intel_drv.h  |   7 +
> > >   drivers/gpu/drm/i915/intel_hdcp.c | 442 
> > > +-
> > >   include/drm/i915_component.h  |  71 ++
> > >   4 files changed, 521 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.h 
> > > b/drivers/gpu/drm/i915/i915_drv.h
> > > index f763b30f98d9..b68bc980b7cd 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > @@ -2015,6 +2015,8 @@ struct drm_i915_private {
> > >   struct i915_pmu pmu;
> > > + struct i915_hdcp_component_master *hdcp_comp;
> > > +
> > >   /*
> > >* NOTE: This is the dri1/ums dungeon, don't add stuff here. 
> > > Your patch
> > >* will be rejected. Instead look for a better place.
> > > diff --git a/drivers/gpu/drm/i915/intel_drv.h 
> > > b/drivers/gpu/drm/i915/intel_drv.h
> > > index 85a526598096..bde82f3ada85 100644
> > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > @@ -29,6 +29,7 @@
> > >   #include 
> > >   #include 
> > >   #include 
> > > +#include 
> > >   #include 
> > >   #include "i915_drv.h"
> > >   #include 
> > > @@ -379,6 +380,9 @@ struct intel_hdcp_shim {
> > >   /* Detects panel's hdcp capability. This is optional for HDMI. 
> > > */
> > >   int (*hdcp_capable)(struct intel_digital_port *intel_dig_port,
> > >   bool *hdcp_capable);
> > > +
> > > + /* Detects the HDCP protocol(DP/HDMI) required on the port */
> > > + enum mei_hdcp_wired_protocol (*hdcp_protocol)(void);
> > Looking ahead, this seems hardwired to constant return value? Or why do we
> > need a function here?
> 
> This is hardwired based on the connector type(DP/HDMI).
> Since we have the shim for hdcp's connector based work, I have added this 
> function.
> 
> Could have done this just with connector_type check, but in that way whole 
> hdcp_shim
> can be done in that way. So going with the larger design here.

If it's hardwired then just make it a hardwired struct member. As long as
it's all const, we can mix data an function pointers. If you have runtime
variable data, then it's better to split it out from the ops structure, so
that we can keep ops read-only and protected against possible exploits
(any function pointers are a high-value target in the kernel).

> 
> > 
> > >   };
> > >   struct intel_hdcp {
> > > @@ -399,6 +403,9 @@ struct intel_hdcp {
> > >* content can flow only through a link protected by HDCP2.2.
> > >*/
> > >   u8 content_type;
> > > +
> > > + /* mei interface related information */
> > > + struct mei_hdcp_data mei_data;
> > >   };
> > >   struct intel_connector {
> > > diff --git a/drivers/gpu/drm/i915/intel_hdcp.c 
> > > b/drivers/gpu/drm/i915/intel_hdcp.c
> > > index 99dddb540958..760780f1105c 100644
> > > --- a/drivers/gpu/drm/i915/intel_hdcp.c
> > > +++ b/drivers/gpu/drm/i915/intel_hdcp.c
> > > @@ -8,14 +8,20 @@
> > >   #include 
> > >   #include 
> > > +#include 
> > >   #include 
> > >   #include 
> > > +#include 
> > >   #include "intel_drv.h"
> > >   #include "i915_reg.h"
> > >   #define KEY_LOAD_TRIES  5
> > >   #define TIME_FOR_ENCRYPT_STATUS_CHANGE  50
> > > +#define GET_MEI_DDI_INDEX(p) ({\
> > > + typeof(p) __p = (p);   \
> > > + __p == PORT_A ? MEI_DDI_A : (enum mei_hdcp_ddi)__p;\
> > > +})
> > >   static
> > >   bool intel_hdcp_is_ksv_valid(u8 *ksv)
> > > @@ -833,6 +839,417 @@ bool is_hdcp_supported(struct drm_i915_private 
> > > *dev_priv, enum port port)
> > >   !IS_CHERRYVIEW(dev_priv) && port < PORT_E);
> > >   }
> > > +static __attribute__((unused)) int
> > > +hdcp2_prepare_ake_init(struct intel_connector *connector,
> > > +struct hdcp2_ake_init *ake_data)
> > > +{
> > > + struct mei_hdcp_data *data = >hdcp.mei_data;
> > > + struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
> > > + struct i915_hdcp_component_master *comp = dev_priv->hdcp_comp;
> > > + int ret;
> > > +
> > > + if (!comp)
> > > + return -EINVAL;
> > > +
> > > + mutex_lock(>mutex);
> > > + if (!comp->ops || !comp->dev) {
> > > + mutex_unlock(>mutex);
> > > + return -EINVAL;
> > > + }
> > > +
> > > + if (data->port == MEI_DDI_INVALID_PORT && connector->encoder)
> > > + data->port = GET_MEI_DDI_INDEX(connector->encoder->port);
> > > +
> > > + /* Clear ME FW instance for the port, just incase */
> > > + 

Re: [Intel-gfx] [PATCH v8 05/35] drm/i915: MEI interface definition

2018-12-07 Thread C, Ramalingam


On 12/7/2018 11:22 AM, C, Ramalingam wrote:



On 12/6/2018 3:53 PM, Daniel Vetter wrote:

On Tue, Nov 27, 2018 at 04:13:03PM +0530, Ramalingam C wrote:

Defining the mei-i915 interface functions and initialization of
the interface.

Signed-off-by: Ramalingam C
Signed-off-by: Tomas Winkler
---
  drivers/gpu/drm/i915/i915_drv.h   |   2 +
  drivers/gpu/drm/i915/intel_drv.h  |   7 +
  drivers/gpu/drm/i915/intel_hdcp.c | 442 +-
  include/drm/i915_component.h  |  71 ++
  4 files changed, 521 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index f763b30f98d9..b68bc980b7cd 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2015,6 +2015,8 @@ struct drm_i915_private {
  
  	struct i915_pmu pmu;
  
+	struct i915_hdcp_component_master *hdcp_comp;

+
/*
 * NOTE: This is the dri1/ums dungeon, don't add stuff here. Your patch
 * will be rejected. Instead look for a better place.
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 85a526598096..bde82f3ada85 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -29,6 +29,7 @@
  #include 
  #include 
  #include 
+#include 
  #include 
  #include "i915_drv.h"
  #include 
@@ -379,6 +380,9 @@ struct intel_hdcp_shim {
/* Detects panel's hdcp capability. This is optional for HDMI. */
int (*hdcp_capable)(struct intel_digital_port *intel_dig_port,
bool *hdcp_capable);
+
+   /* Detects the HDCP protocol(DP/HDMI) required on the port */
+   enum mei_hdcp_wired_protocol (*hdcp_protocol)(void);

Looking ahead, this seems hardwired to constant return value? Or why do we
need a function here?

This is hardwired based on the connector type(DP/HDMI).
Since we have the shim for hdcp's connector based work, I have added this 
function.

Could have done this just with connector_type check, but in that way whole 
hdcp_shim
can be done in that way. So going with the larger design here.

  };
  
  struct intel_hdcp {

@@ -399,6 +403,9 @@ struct intel_hdcp {
 * content can flow only through a link protected by HDCP2.2.
 */
u8 content_type;
+
+   /* mei interface related information */
+   struct mei_hdcp_data mei_data;
  };
  
  struct intel_connector {

diff --git a/drivers/gpu/drm/i915/intel_hdcp.c 
b/drivers/gpu/drm/i915/intel_hdcp.c
index 99dddb540958..760780f1105c 100644
--- a/drivers/gpu/drm/i915/intel_hdcp.c
+++ b/drivers/gpu/drm/i915/intel_hdcp.c
@@ -8,14 +8,20 @@
  
  #include 

  #include 
+#include 
  #include 
  #include 
+#include 
  
  #include "intel_drv.h"

  #include "i915_reg.h"
  
  #define KEY_LOAD_TRIES	5

  #define TIME_FOR_ENCRYPT_STATUS_CHANGE50
+#define GET_MEI_DDI_INDEX(p) ({\
+   typeof(p) __p = (p);   \
+   __p == PORT_A ? MEI_DDI_A : (enum mei_hdcp_ddi)__p;\
+})
  
  static

  bool intel_hdcp_is_ksv_valid(u8 *ksv)
@@ -833,6 +839,417 @@ bool is_hdcp_supported(struct drm_i915_private *dev_priv, 
enum port port)
!IS_CHERRYVIEW(dev_priv) && port < PORT_E);
  }
  
+static __attribute__((unused)) int

+hdcp2_prepare_ake_init(struct intel_connector *connector,
+  struct hdcp2_ake_init *ake_data)
+{
+   struct mei_hdcp_data *data = >hdcp.mei_data;
+   struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
+   struct i915_hdcp_component_master *comp = dev_priv->hdcp_comp;
+   int ret;
+
+   if (!comp)
+   return -EINVAL;
+
+   mutex_lock(>mutex);
+   if (!comp->ops || !comp->dev) {
+   mutex_unlock(>mutex);
+   return -EINVAL;
+   }
+
+   if (data->port == MEI_DDI_INVALID_PORT && connector->encoder)
+   data->port = GET_MEI_DDI_INDEX(connector->encoder->port);
+
+   /* Clear ME FW instance for the port, just incase */
+   comp->ops->close_hdcp_session(comp->dev, data);

Sounds like a bug somewhere if we need this? I'd put this code into the
->initiate_hdcp2_session, with a big WARN_ON if it's actually needed.

Not really. Lets say you have progressed beyond the first step of HDCP2.2 auth 
along with ME FW.
Now if authentication failed due to some reason, then the HDCP2.2 season 
created with
ME FW for that port is not closed yet.

So we need to call close_hdcp_session() explicitly on authentication failures.
Session has to be closed before restarting the auth on that port with 
initialite_hdcp_session.
If we are closing the hdcp session of the port on all auth errors, we dont need 
this just before
start of the hdcp session.

"Just in case" papering over programming bugs of our own just makes
debugging harder.


+
+   ret = comp->ops->initiate_hdcp2_session(comp->dev,
+   data, ake_data);

We should set 

Re: [Intel-gfx] [PATCH v8 05/35] drm/i915: MEI interface definition

2018-12-07 Thread C, Ramalingam


On 12/7/2018 11:22 AM, C, Ramalingam wrote:



On 12/6/2018 3:53 PM, Daniel Vetter wrote:

On Tue, Nov 27, 2018 at 04:13:03PM +0530, Ramalingam C wrote:

Defining the mei-i915 interface functions and initialization of
the interface.

Signed-off-by: Ramalingam C
Signed-off-by: Tomas Winkler
---
  drivers/gpu/drm/i915/i915_drv.h   |   2 +
  drivers/gpu/drm/i915/intel_drv.h  |   7 +
  drivers/gpu/drm/i915/intel_hdcp.c | 442 +-
  include/drm/i915_component.h  |  71 ++
  4 files changed, 521 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index f763b30f98d9..b68bc980b7cd 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2015,6 +2015,8 @@ struct drm_i915_private {
  
  	struct i915_pmu pmu;
  
+	struct i915_hdcp_component_master *hdcp_comp;

+
/*
 * NOTE: This is the dri1/ums dungeon, don't add stuff here. Your patch
 * will be rejected. Instead look for a better place.
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 85a526598096..bde82f3ada85 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -29,6 +29,7 @@
  #include 
  #include 
  #include 
+#include 
  #include 
  #include "i915_drv.h"
  #include 
@@ -379,6 +380,9 @@ struct intel_hdcp_shim {
/* Detects panel's hdcp capability. This is optional for HDMI. */
int (*hdcp_capable)(struct intel_digital_port *intel_dig_port,
bool *hdcp_capable);
+
+   /* Detects the HDCP protocol(DP/HDMI) required on the port */
+   enum mei_hdcp_wired_protocol (*hdcp_protocol)(void);

Looking ahead, this seems hardwired to constant return value? Or why do we
need a function here?

This is hardwired based on the connector type(DP/HDMI).
Since we have the shim for hdcp's connector based work, I have added this 
function.

Could have done this just with connector_type check, but in that way whole 
hdcp_shim
can be done in that way. So going with the larger design here.

  };
  
  struct intel_hdcp {

@@ -399,6 +403,9 @@ struct intel_hdcp {
 * content can flow only through a link protected by HDCP2.2.
 */
u8 content_type;
+
+   /* mei interface related information */
+   struct mei_hdcp_data mei_data;
  };
  
  struct intel_connector {

diff --git a/drivers/gpu/drm/i915/intel_hdcp.c 
b/drivers/gpu/drm/i915/intel_hdcp.c
index 99dddb540958..760780f1105c 100644
--- a/drivers/gpu/drm/i915/intel_hdcp.c
+++ b/drivers/gpu/drm/i915/intel_hdcp.c
@@ -8,14 +8,20 @@
  
  #include 

  #include 
+#include 
  #include 
  #include 
+#include 
  
  #include "intel_drv.h"

  #include "i915_reg.h"
  
  #define KEY_LOAD_TRIES	5

  #define TIME_FOR_ENCRYPT_STATUS_CHANGE50
+#define GET_MEI_DDI_INDEX(p) ({\
+   typeof(p) __p = (p);   \
+   __p == PORT_A ? MEI_DDI_A : (enum mei_hdcp_ddi)__p;\
+})
  
  static

  bool intel_hdcp_is_ksv_valid(u8 *ksv)
@@ -833,6 +839,417 @@ bool is_hdcp_supported(struct drm_i915_private *dev_priv, 
enum port port)
!IS_CHERRYVIEW(dev_priv) && port < PORT_E);
  }
  
+static __attribute__((unused)) int

+hdcp2_prepare_ake_init(struct intel_connector *connector,
+  struct hdcp2_ake_init *ake_data)
+{
+   struct mei_hdcp_data *data = >hdcp.mei_data;
+   struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
+   struct i915_hdcp_component_master *comp = dev_priv->hdcp_comp;
+   int ret;
+
+   if (!comp)
+   return -EINVAL;
+
+   mutex_lock(>mutex);
+   if (!comp->ops || !comp->dev) {
+   mutex_unlock(>mutex);
+   return -EINVAL;
+   }
+
+   if (data->port == MEI_DDI_INVALID_PORT && connector->encoder)
+   data->port = GET_MEI_DDI_INDEX(connector->encoder->port);
+
+   /* Clear ME FW instance for the port, just incase */
+   comp->ops->close_hdcp_session(comp->dev, data);

Sounds like a bug somewhere if we need this? I'd put this code into the
->initiate_hdcp2_session, with a big WARN_ON if it's actually needed.

Not really. Lets say you have progressed beyond the first step of HDCP2.2 auth 
along with ME FW.
Now if authentication failed due to some reason, then the HDCP2.2 season 
created with
ME FW for that port is not closed yet.

So we need to call close_hdcp_session() explicitly on authentication failures.
Session has to be closed before restarting the auth on that port with 
initialite_hdcp_session.
If we are closing the hdcp session of the port on all auth errors, we dont need 
this just before
start of the hdcp session.

"Just in case" papering over programming bugs of our own just makes
debugging harder.


+
+   ret = comp->ops->initiate_hdcp2_session(comp->dev,
+   data, ake_data);

We should set 

Re: [Intel-gfx] [PATCH v8 05/35] drm/i915: MEI interface definition

2018-12-06 Thread C, Ramalingam


On 12/6/2018 3:53 PM, Daniel Vetter wrote:

On Tue, Nov 27, 2018 at 04:13:03PM +0530, Ramalingam C wrote:

Defining the mei-i915 interface functions and initialization of
the interface.

Signed-off-by: Ramalingam C 
Signed-off-by: Tomas Winkler 
---
  drivers/gpu/drm/i915/i915_drv.h   |   2 +
  drivers/gpu/drm/i915/intel_drv.h  |   7 +
  drivers/gpu/drm/i915/intel_hdcp.c | 442 +-
  include/drm/i915_component.h  |  71 ++
  4 files changed, 521 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index f763b30f98d9..b68bc980b7cd 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2015,6 +2015,8 @@ struct drm_i915_private {
  
  	struct i915_pmu pmu;
  
+	struct i915_hdcp_component_master *hdcp_comp;

+
/*
 * NOTE: This is the dri1/ums dungeon, don't add stuff here. Your patch
 * will be rejected. Instead look for a better place.
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 85a526598096..bde82f3ada85 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -29,6 +29,7 @@
  #include 
  #include 
  #include 
+#include 
  #include 
  #include "i915_drv.h"
  #include 
@@ -379,6 +380,9 @@ struct intel_hdcp_shim {
/* Detects panel's hdcp capability. This is optional for HDMI. */
int (*hdcp_capable)(struct intel_digital_port *intel_dig_port,
bool *hdcp_capable);
+
+   /* Detects the HDCP protocol(DP/HDMI) required on the port */
+   enum mei_hdcp_wired_protocol (*hdcp_protocol)(void);

Looking ahead, this seems hardwired to constant return value? Or why do we
need a function here?


This is hardwired based on the connector type(DP/HDMI).
Since we have the shim for hdcp's connector based work, I have added this 
function.

Could have done this just with connector_type check, but in that way whole 
hdcp_shim
can be done in that way. So going with the larger design here.




  };
  
  struct intel_hdcp {

@@ -399,6 +403,9 @@ struct intel_hdcp {
 * content can flow only through a link protected by HDCP2.2.
 */
u8 content_type;
+
+   /* mei interface related information */
+   struct mei_hdcp_data mei_data;
  };
  
  struct intel_connector {

diff --git a/drivers/gpu/drm/i915/intel_hdcp.c 
b/drivers/gpu/drm/i915/intel_hdcp.c
index 99dddb540958..760780f1105c 100644
--- a/drivers/gpu/drm/i915/intel_hdcp.c
+++ b/drivers/gpu/drm/i915/intel_hdcp.c
@@ -8,14 +8,20 @@
  
  #include 

  #include 
+#include 
  #include 
  #include 
+#include 
  
  #include "intel_drv.h"

  #include "i915_reg.h"
  
  #define KEY_LOAD_TRIES	5

  #define TIME_FOR_ENCRYPT_STATUS_CHANGE50
+#define GET_MEI_DDI_INDEX(p) ({\
+   typeof(p) __p = (p);   \
+   __p == PORT_A ? MEI_DDI_A : (enum mei_hdcp_ddi)__p;\
+})
  
  static

  bool intel_hdcp_is_ksv_valid(u8 *ksv)
@@ -833,6 +839,417 @@ bool is_hdcp_supported(struct drm_i915_private *dev_priv, 
enum port port)
!IS_CHERRYVIEW(dev_priv) && port < PORT_E);
  }
  
+static __attribute__((unused)) int

+hdcp2_prepare_ake_init(struct intel_connector *connector,
+  struct hdcp2_ake_init *ake_data)
+{
+   struct mei_hdcp_data *data = >hdcp.mei_data;
+   struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
+   struct i915_hdcp_component_master *comp = dev_priv->hdcp_comp;
+   int ret;
+
+   if (!comp)
+   return -EINVAL;
+
+   mutex_lock(>mutex);
+   if (!comp->ops || !comp->dev) {
+   mutex_unlock(>mutex);
+   return -EINVAL;
+   }
+
+   if (data->port == MEI_DDI_INVALID_PORT && connector->encoder)
+   data->port = GET_MEI_DDI_INDEX(connector->encoder->port);
+
+   /* Clear ME FW instance for the port, just incase */
+   comp->ops->close_hdcp_session(comp->dev, data);

Sounds like a bug somewhere if we need this? I'd put this code into the
->initiate_hdcp2_session, with a big WARN_ON if it's actually needed.


Not really. Lets say you have progressed beyond the first step of HDCP2.2 auth 
along with ME FW.
Now if authentication failed due to some reason, then the HDCP2.2 season 
created with
ME FW for that port is not closed yet.

So we need to call close_hdcp_session() explicitly on authentication failures.
Session has to be closed before restarting the auth on that port with 
initialite_hdcp_session.
If we are closing the hdcp session of the port on all auth errors, we dont need 
this just before
start of the hdcp session.



"Just in case" papering over programming bugs of our own just makes
debugging harder.


+
+   ret = comp->ops->initiate_hdcp2_session(comp->dev,
+   data, ake_data);

We should set the port only after successfully 

Re: [Intel-gfx] [PATCH v8 05/35] drm/i915: MEI interface definition

2018-12-06 Thread Daniel Vetter
On Tue, Nov 27, 2018 at 04:13:03PM +0530, Ramalingam C wrote:
> Defining the mei-i915 interface functions and initialization of
> the interface.
> 
> Signed-off-by: Ramalingam C 
> Signed-off-by: Tomas Winkler 
> ---
>  drivers/gpu/drm/i915/i915_drv.h   |   2 +
>  drivers/gpu/drm/i915/intel_drv.h  |   7 +
>  drivers/gpu/drm/i915/intel_hdcp.c | 442 
> +-
>  include/drm/i915_component.h  |  71 ++
>  4 files changed, 521 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index f763b30f98d9..b68bc980b7cd 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2015,6 +2015,8 @@ struct drm_i915_private {
>  
>   struct i915_pmu pmu;
>  
> + struct i915_hdcp_component_master *hdcp_comp;
> +
>   /*
>* NOTE: This is the dri1/ums dungeon, don't add stuff here. Your patch
>* will be rejected. Instead look for a better place.
> diff --git a/drivers/gpu/drm/i915/intel_drv.h 
> b/drivers/gpu/drm/i915/intel_drv.h
> index 85a526598096..bde82f3ada85 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -29,6 +29,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include "i915_drv.h"
>  #include 
> @@ -379,6 +380,9 @@ struct intel_hdcp_shim {
>   /* Detects panel's hdcp capability. This is optional for HDMI. */
>   int (*hdcp_capable)(struct intel_digital_port *intel_dig_port,
>   bool *hdcp_capable);
> +
> + /* Detects the HDCP protocol(DP/HDMI) required on the port */
> + enum mei_hdcp_wired_protocol (*hdcp_protocol)(void);

Looking ahead, this seems hardwired to constant return value? Or why do we
need a function here?

>  };
>  
>  struct intel_hdcp {
> @@ -399,6 +403,9 @@ struct intel_hdcp {
>* content can flow only through a link protected by HDCP2.2.
>*/
>   u8 content_type;
> +
> + /* mei interface related information */
> + struct mei_hdcp_data mei_data;
>  };
>  
>  struct intel_connector {
> diff --git a/drivers/gpu/drm/i915/intel_hdcp.c 
> b/drivers/gpu/drm/i915/intel_hdcp.c
> index 99dddb540958..760780f1105c 100644
> --- a/drivers/gpu/drm/i915/intel_hdcp.c
> +++ b/drivers/gpu/drm/i915/intel_hdcp.c
> @@ -8,14 +8,20 @@
>  
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
> +#include 
>  
>  #include "intel_drv.h"
>  #include "i915_reg.h"
>  
>  #define KEY_LOAD_TRIES   5
>  #define TIME_FOR_ENCRYPT_STATUS_CHANGE   50
> +#define GET_MEI_DDI_INDEX(p) ({\
> + typeof(p) __p = (p);   \
> + __p == PORT_A ? MEI_DDI_A : (enum mei_hdcp_ddi)__p;\
> +})
>  
>  static
>  bool intel_hdcp_is_ksv_valid(u8 *ksv)
> @@ -833,6 +839,417 @@ bool is_hdcp_supported(struct drm_i915_private 
> *dev_priv, enum port port)
>   !IS_CHERRYVIEW(dev_priv) && port < PORT_E);
>  }
>  
> +static __attribute__((unused)) int
> +hdcp2_prepare_ake_init(struct intel_connector *connector,
> +struct hdcp2_ake_init *ake_data)
> +{
> + struct mei_hdcp_data *data = >hdcp.mei_data;
> + struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
> + struct i915_hdcp_component_master *comp = dev_priv->hdcp_comp;
> + int ret;
> +
> + if (!comp)
> + return -EINVAL;
> +
> + mutex_lock(>mutex);
> + if (!comp->ops || !comp->dev) {
> + mutex_unlock(>mutex);
> + return -EINVAL;
> + }
> +
> + if (data->port == MEI_DDI_INVALID_PORT && connector->encoder)
> + data->port = GET_MEI_DDI_INDEX(connector->encoder->port);
> +
> + /* Clear ME FW instance for the port, just incase */
> + comp->ops->close_hdcp_session(comp->dev, data);

Sounds like a bug somewhere if we need this? I'd put this code into the
->initiate_hdcp2_session, with a big WARN_ON if it's actually needed.

"Just in case" papering over programming bugs of our own just makes
debugging harder.

> +
> + ret = comp->ops->initiate_hdcp2_session(comp->dev,
> + data, ake_data);

We should set the port only after successfully initializing this.

> + mutex_unlock(>mutex);
> +
> + return ret;
> +}
> +
> +static __attribute__((unused)) int
> +hdcp2_verify_rx_cert_prepare_km(struct intel_connector *connector,
> + struct hdcp2_ake_send_cert *rx_cert,
> + bool *paired,
> + struct hdcp2_ake_no_stored_km *ek_pub_km,
> + size_t *msg_sz)
> +{
> + struct mei_hdcp_data *data = >hdcp.mei_data;
> + struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
> + struct i915_hdcp_component_master *comp = dev_priv->hdcp_comp;
> + int ret;
> +
> + if (!comp)
> + return -EINVAL;
> +
> + mutex_lock(>mutex);
> + if 

[Intel-gfx] [PATCH v8 05/35] drm/i915: MEI interface definition

2018-11-27 Thread Ramalingam C
Defining the mei-i915 interface functions and initialization of
the interface.

Signed-off-by: Ramalingam C 
Signed-off-by: Tomas Winkler 
---
 drivers/gpu/drm/i915/i915_drv.h   |   2 +
 drivers/gpu/drm/i915/intel_drv.h  |   7 +
 drivers/gpu/drm/i915/intel_hdcp.c | 442 +-
 include/drm/i915_component.h  |  71 ++
 4 files changed, 521 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index f763b30f98d9..b68bc980b7cd 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2015,6 +2015,8 @@ struct drm_i915_private {
 
struct i915_pmu pmu;
 
+   struct i915_hdcp_component_master *hdcp_comp;
+
/*
 * NOTE: This is the dri1/ums dungeon, don't add stuff here. Your patch
 * will be rejected. Instead look for a better place.
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 85a526598096..bde82f3ada85 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -29,6 +29,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include "i915_drv.h"
 #include 
@@ -379,6 +380,9 @@ struct intel_hdcp_shim {
/* Detects panel's hdcp capability. This is optional for HDMI. */
int (*hdcp_capable)(struct intel_digital_port *intel_dig_port,
bool *hdcp_capable);
+
+   /* Detects the HDCP protocol(DP/HDMI) required on the port */
+   enum mei_hdcp_wired_protocol (*hdcp_protocol)(void);
 };
 
 struct intel_hdcp {
@@ -399,6 +403,9 @@ struct intel_hdcp {
 * content can flow only through a link protected by HDCP2.2.
 */
u8 content_type;
+
+   /* mei interface related information */
+   struct mei_hdcp_data mei_data;
 };
 
 struct intel_connector {
diff --git a/drivers/gpu/drm/i915/intel_hdcp.c 
b/drivers/gpu/drm/i915/intel_hdcp.c
index 99dddb540958..760780f1105c 100644
--- a/drivers/gpu/drm/i915/intel_hdcp.c
+++ b/drivers/gpu/drm/i915/intel_hdcp.c
@@ -8,14 +8,20 @@
 
 #include 
 #include 
+#include 
 #include 
 #include 
+#include 
 
 #include "intel_drv.h"
 #include "i915_reg.h"
 
 #define KEY_LOAD_TRIES 5
 #define TIME_FOR_ENCRYPT_STATUS_CHANGE 50
+#define GET_MEI_DDI_INDEX(p) ({\
+   typeof(p) __p = (p);   \
+   __p == PORT_A ? MEI_DDI_A : (enum mei_hdcp_ddi)__p;\
+})
 
 static
 bool intel_hdcp_is_ksv_valid(u8 *ksv)
@@ -833,6 +839,417 @@ bool is_hdcp_supported(struct drm_i915_private *dev_priv, 
enum port port)
!IS_CHERRYVIEW(dev_priv) && port < PORT_E);
 }
 
+static __attribute__((unused)) int
+hdcp2_prepare_ake_init(struct intel_connector *connector,
+  struct hdcp2_ake_init *ake_data)
+{
+   struct mei_hdcp_data *data = >hdcp.mei_data;
+   struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
+   struct i915_hdcp_component_master *comp = dev_priv->hdcp_comp;
+   int ret;
+
+   if (!comp)
+   return -EINVAL;
+
+   mutex_lock(>mutex);
+   if (!comp->ops || !comp->dev) {
+   mutex_unlock(>mutex);
+   return -EINVAL;
+   }
+
+   if (data->port == MEI_DDI_INVALID_PORT && connector->encoder)
+   data->port = GET_MEI_DDI_INDEX(connector->encoder->port);
+
+   /* Clear ME FW instance for the port, just incase */
+   comp->ops->close_hdcp_session(comp->dev, data);
+
+   ret = comp->ops->initiate_hdcp2_session(comp->dev,
+   data, ake_data);
+   mutex_unlock(>mutex);
+
+   return ret;
+}
+
+static __attribute__((unused)) int
+hdcp2_verify_rx_cert_prepare_km(struct intel_connector *connector,
+   struct hdcp2_ake_send_cert *rx_cert,
+   bool *paired,
+   struct hdcp2_ake_no_stored_km *ek_pub_km,
+   size_t *msg_sz)
+{
+   struct mei_hdcp_data *data = >hdcp.mei_data;
+   struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
+   struct i915_hdcp_component_master *comp = dev_priv->hdcp_comp;
+   int ret;
+
+   if (!comp)
+   return -EINVAL;
+
+   mutex_lock(>mutex);
+   if (!comp->ops || !comp->dev || data->port == MEI_DDI_INVALID_PORT) {
+   mutex_unlock(>mutex);
+   return -EINVAL;
+   }
+
+   ret = comp->ops->verify_receiver_cert_prepare_km(comp->dev, data,
+rx_cert, paired,
+ek_pub_km, msg_sz);
+   if (ret < 0)
+   comp->ops->close_hdcp_session(comp->dev, data);
+   mutex_unlock(>mutex);
+
+   return ret;
+}
+
+static __attribute__((unused)) int
+hdcp2_verify_hprime(struct intel_connector *connector,
+   struct