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@xxxxxxxxx>
> > ---
> > 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
