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 at 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.
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