Re: [PATCH v10 18/40] drm/i915: CP_IRQ handling for DP HDCP2.2 msgs

[Daniel Vetter <daniel@ffwll.ch>]

On Thu, Jan 31, 2019 at 09:08:04AM +0100, Daniel Vetter wrote:
> On Thu, Jan 31, 2019 at 12:29:34PM +0530, Ramalingam C wrote:
> > Implements the
> > 	Waitqueue is created to wait for CP_IRQ
> > 	Signaling the CP_IRQ arrival through atomic variable.
> > 	For applicable DP HDCP2.2 msgs read wait for CP_IRQ.
> > 
> > As per HDCP2.2 spec "HDCP Transmitters must process CP_IRQ interrupts
> > when they are received from HDCP Receivers"
> > 
> > Without CP_IRQ processing, DP HDCP2.2 H_Prime msg was getting corrupted
> > while reading it based on corresponding status bit. This creates the
> > random failures in reading the DP HDCP2.2 msgs.
> > 
> > Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_dp.c   | 33 +++++++++++++++++++++++++--------
> >  drivers/gpu/drm/i915/intel_drv.h  |  7 +++++++
> >  drivers/gpu/drm/i915/intel_hdcp.c |  6 ++++++
> >  3 files changed, 38 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index b13c41220ce0..4e36df266ab3 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -5619,6 +5619,24 @@ void intel_dp_encoder_suspend(struct intel_encoder *intel_encoder)
> >  		edp_panel_vdd_off_sync(intel_dp);
> >  }
> >  
> > +static void intel_dp_hdcp_wait_for_cp_irq(struct intel_hdcp *hdcp,
> > +					  int timeout)
> > +{
> > +	long ret;
> > +
> > +	/* Reinit */
> > +	atomic_set(&hdcp->cp_irq_recved, 0);
> 
> This is still fundamentally racy. The way it's usually done is through a
> counter, i.e. the wait function samples the atomic counter, and the wait
> condition is then that the counter has increased.
> 
> The interrupt handler on the other hand just does an atomic_inc. That way
> you never have the problem that the interrupt handler has set 1 before we
> clear it to 0 here.
> 
> That still leaves the race that it has incremented _before_ we sampled in
> this function here. There the counter sampling needs to be moved out
> (probably before we send out the message to the sink), and passed into
> this function as a parameter.
> 
> Finally there's the wrapping problem, so best to just have a condition
> like sampled_counter != current_counter.
> 
> I assume this won't matter for correctness if we miss the interrupt, we
> just time out and at that point the next message should be available. But
> it's less confusing to have more correct code.

Another option would be to shrug the races off (add a comment explaining
why that's ok) and use struct completion instead of wait_queue. That one
is explicitly for one-shot stuff where the wake-up itself is all we need,
and there's no further condition to check.
-Daniel

> 
> Cheers, Daniel
> 
> > +
> > +#define C (atomic_read(&hdcp->cp_irq_recved) > 0)
> > +	ret = wait_event_interruptible_timeout(hdcp->cp_irq_queue, C,
> > +					       msecs_to_jiffies(timeout));
> > +
> > +	if (ret > 0)
> > +		atomic_set(&hdcp->cp_irq_recved, 0);
> > +	else if (!ret)
> > +		DRM_DEBUG_KMS("Timedout at waiting for CP_IRQ\n");
> > +}
> > +
> >  static
> >  int intel_dp_hdcp_write_an_aksv(struct intel_digital_port *intel_dig_port,
> >  				u8 *an)
> > @@ -5963,14 +5981,13 @@ intel_dp_hdcp2_wait_for_msg(struct intel_digital_port *intel_dig_port,
> >  		mdelay(timeout);
> >  		ret = 0;
> >  	} else {
> > -		/* TODO: In case if you need to wait on CP_IRQ, do it here */
> > -		ret = __wait_for(ret =
> > -				 hdcp2_detect_msg_availability(intel_dig_port,
> > -							       msg_id,
> > -							       &msg_ready),
> > -				 !ret && msg_ready, timeout * 1000,
> > -				 1000, 5 * 1000);
> > -
> > +		/*
> > +		 * Ignoring the return of the intel_dp_hdcp_wait_for_cp_irq,
> > +		 * Just to detect the msg availability before failing it.
> > +		 */
> > +		intel_dp_hdcp_wait_for_cp_irq(hdcp, timeout);
> > +		ret = hdcp2_detect_msg_availability(intel_dig_port,
> > +						    msg_id, &msg_ready);
> >  		if (!msg_ready)
> >  			ret = -ETIMEDOUT;
> >  	}
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index 747fe7361287..1901d12bacc4 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -464,6 +464,13 @@ struct intel_hdcp {
> >  	 * over re-Auth has to be triggered.
> >  	 */
> >  	u32 seq_num_m;
> > +
> > +	/*
> > +	 * Work queue to signal the CP_IRQ. Used for the waiters to read the
> > +	 * available information from HDCP DP sink.
> > +	 */
> > +	wait_queue_head_t cp_irq_queue;
> > +	atomic_t cp_irq_recved;
> >  };
> >  
> >  struct intel_connector {
> > diff --git a/drivers/gpu/drm/i915/intel_hdcp.c b/drivers/gpu/drm/i915/intel_hdcp.c
> > index 7ff29fb0aa2f..f38feeadb363 100644
> > --- a/drivers/gpu/drm/i915/intel_hdcp.c
> > +++ b/drivers/gpu/drm/i915/intel_hdcp.c
> > @@ -1805,6 +1805,9 @@ int intel_hdcp_init(struct intel_connector *connector,
> >  	if (hdcp2_supported)
> >  		intel_hdcp2_init(connector);
> >  
> > +	atomic_set(&hdcp->cp_irq_recved, 0);
> > +	init_waitqueue_head(&hdcp->cp_irq_queue);
> > +
> >  	return 0;
> >  }
> >  
> > @@ -1908,6 +1911,9 @@ void intel_hdcp_handle_cp_irq(struct intel_connector *connector)
> >  	if (!hdcp->shim)
> >  		return;
> >  
> > +	atomic_set(&connector->hdcp.cp_irq_recved, 1);
> > +	wake_up_all(&connector->hdcp.cp_irq_queue);
> > +
> >  	/*
> >  	 * CP_IRQ could be triggered due to 1. HDCP2.2 auth msgs availability,
> >  	 * 2. link failure and 3. repeater reauth request. At present we dont
> > -- 
> > 2.7.4
> > 
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

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