| Message ID | 1519665159-28639-6-git-send-email-ramalingam.c@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Feb 26, 2018 at 10:42:39PM +0530, Ramalingam C wrote: > DP and HDMI HDCP specifications are varying with respect to > detecting the R0 and ksv_fifo availability. > > DP will produce CP_IRQ and set a bit for indicating the R0 and > FIFO_READY status. I'm not sure what the benefit is? Keeping things common was a deliberate decision on my part to keep complexity down and increase the amount of common code. Sean > > Whereas HDMI will set a bit for FIFO_READY but there is no bit > indication for R0 ready. And also polling of READY bit is needed for > HDMI as ther is no CP_IRQ. > > So Fielding the CP_IRQ for DP and the polling the READY bit for a > period and blindly waiting for a fixed time for R0 incase of HDMI are > moved into corresponding hdcp_shim. > > Signed-off-by: Ramalingam C <ramalingam.c@intel.com> > --- > drivers/gpu/drm/i915/intel_dp.c | 70 +++++++++++++++++++++++++++++++++++---- > drivers/gpu/drm/i915/intel_drv.h | 6 ++-- > drivers/gpu/drm/i915/intel_hdcp.c | 39 ++-------------------- > drivers/gpu/drm/i915/intel_hdmi.c | 28 +++++++++++++++- > 4 files changed, 98 insertions(+), 45 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index 2c3eb90b9499..afe310a91d3c 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -4436,6 +4436,7 @@ static bool > intel_dp_short_pulse(struct intel_dp *intel_dp) > { > struct drm_i915_private *dev_priv = to_i915(intel_dp_to_dev(intel_dp)); > + struct intel_connector *connector = intel_dp->attached_connector; > u8 sink_irq_vector = 0; > u8 old_sink_count = intel_dp->sink_count; > bool ret; > @@ -4470,8 +4471,11 @@ intel_dp_short_pulse(struct intel_dp *intel_dp) > > if (sink_irq_vector & DP_AUTOMATED_TEST_REQUEST) > intel_dp_handle_test_request(intel_dp); > - if (sink_irq_vector & (DP_CP_IRQ | DP_SINK_SPECIFIC_IRQ)) > - DRM_DEBUG_DRIVER("CP or sink specific irq unhandled\n"); > + if (sink_irq_vector & DP_CP_IRQ) > + if (connector->hdcp_shim) > + complete_all(&connector->cp_irq_recved); > + if (sink_irq_vector & DP_SINK_SPECIFIC_IRQ) > + DRM_DEBUG_DRIVER("Sink specific irq unhandled\n"); > } > > intel_dp_check_link_status(intel_dp); > @@ -5083,6 +5087,25 @@ void intel_dp_encoder_suspend(struct intel_encoder *intel_encoder) > pps_unlock(intel_dp); > } > > +static int intel_dp_hdcp_wait_for_cp_irq(struct completion *cp_irq_recved, > + int timeout) > +{ > + long ret; > + > + if (completion_done(cp_irq_recved)) > + reinit_completion(cp_irq_recved); > + > + ret = wait_for_completion_interruptible_timeout(cp_irq_recved, > + msecs_to_jiffies( > + timeout)); > + reinit_completion(cp_irq_recved); > + if (ret < 0) > + return (int)ret; > + else if (!ret) > + return -ETIMEDOUT; > + return 0; > +} > + > static > int intel_dp_hdcp_write_an_aksv(struct intel_digital_port *intel_dig_port, > u8 *an) > @@ -5188,11 +5211,38 @@ int intel_dp_hdcp_repeater_present(struct intel_digital_port *intel_dig_port, > return 0; > } > > +static int intel_dp_hdcp_wait_for_r0(struct intel_digital_port *intel_dig_port) > +{ > + struct intel_dp *dp = &intel_dig_port->dp; > + struct intel_connector *connector = dp->attached_connector; > + int ret; > + u8 bstatus; > + > + intel_dp_hdcp_wait_for_cp_irq(&connector->cp_irq_recved, 100); > + > + ret = drm_dp_dpcd_read(&intel_dig_port->dp.aux, DP_AUX_HDCP_BSTATUS, > + &bstatus, 1); > + if (ret != 1) { > + DRM_ERROR("Read bstatus from DP/AUX failed (%d)\n", ret); > + return ret >= 0 ? -EIO : ret; > + } > + > + if (!(bstatus & DP_BSTATUS_R0_PRIME_READY)) > + return -ETIMEDOUT; > + > + return 0; > +} > + > static > int intel_dp_hdcp_read_ri_prime(struct intel_digital_port *intel_dig_port, > u8 *ri_prime) > { > ssize_t ret; > + > + ret = intel_dp_hdcp_wait_for_r0(intel_dig_port); > + if (!ret) > + return ret; > + > ret = drm_dp_dpcd_read(&intel_dig_port->dp.aux, DP_AUX_HDCP_RI_PRIME, > ri_prime, DRM_HDCP_RI_LEN); > if (ret != DRM_HDCP_RI_LEN) { > @@ -5203,18 +5253,26 @@ int intel_dp_hdcp_read_ri_prime(struct intel_digital_port *intel_dig_port, > } > > static > -int intel_dp_hdcp_read_ksv_ready(struct intel_digital_port *intel_dig_port, > - bool *ksv_ready) > +int intel_dp_hdcp_ksv_ready(struct intel_digital_port *intel_dig_port) > { > + struct intel_dp *dp = &intel_dig_port->dp; > + struct intel_connector *connector = dp->attached_connector; > ssize_t ret; > u8 bstatus; > + > + intel_dp_hdcp_wait_for_cp_irq(&connector->cp_irq_recved, > + 5 * 1000 * 1000); > + > ret = drm_dp_dpcd_read(&intel_dig_port->dp.aux, DP_AUX_HDCP_BSTATUS, > &bstatus, 1); > if (ret != 1) { > DRM_ERROR("Read bstatus from DP/AUX failed (%zd)\n", ret); > return ret >= 0 ? -EIO : ret; > } > - *ksv_ready = bstatus & DP_BSTATUS_READY; > + > + if (!(bstatus & DP_BSTATUS_READY)) > + return -ETIMEDOUT; > + > return 0; > } > > @@ -5305,7 +5363,7 @@ static const struct intel_hdcp_shim intel_dp_hdcp_shim = { > .read_bstatus = intel_dp_hdcp_read_bstatus, > .repeater_present = intel_dp_hdcp_repeater_present, > .read_ri_prime = intel_dp_hdcp_read_ri_prime, > - .read_ksv_ready = intel_dp_hdcp_read_ksv_ready, > + .wait_for_ksv_ready = intel_dp_hdcp_ksv_ready, > .read_ksv_fifo = intel_dp_hdcp_read_ksv_fifo, > .read_v_prime_part = intel_dp_hdcp_read_v_prime_part, > .toggle_signalling = intel_dp_hdcp_toggle_signalling, > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index 8f38e584d375..ab975107c978 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -353,8 +353,7 @@ struct intel_hdcp_shim { > int (*read_ri_prime)(struct intel_digital_port *intel_dig_port, u8 *ri); > > /* Determines if the receiver's KSV FIFO is ready for consumption */ > - int (*read_ksv_ready)(struct intel_digital_port *intel_dig_port, > - bool *ksv_ready); > + int (*wait_for_ksv_ready)(struct intel_digital_port *intel_dig_port); > > /* Reads the ksv fifo for num_downstream devices */ > int (*read_ksv_fifo)(struct intel_digital_port *intel_dig_port, > @@ -413,6 +412,9 @@ struct intel_connector { > uint64_t hdcp_value; /* protected by hdcp_mutex */ > struct delayed_work hdcp_check_work; > struct work_struct hdcp_prop_work; > + > + /* To indicate the assertion of CP_IRQ */ > + struct completion cp_irq_recved; > }; > > struct intel_digital_connector_state { > diff --git a/drivers/gpu/drm/i915/intel_hdcp.c b/drivers/gpu/drm/i915/intel_hdcp.c > index 14be14a45e5a..a077e1333886 100644 > --- a/drivers/gpu/drm/i915/intel_hdcp.c > +++ b/drivers/gpu/drm/i915/intel_hdcp.c > @@ -57,27 +57,6 @@ static bool wait_for_hdcp_port(struct intel_connector *connector) > return true; > } > > -static int intel_hdcp_poll_ksv_fifo(struct intel_digital_port *intel_dig_port, > - const struct intel_hdcp_shim *shim) > -{ > - int ret, read_ret; > - bool ksv_ready; > - > - /* Poll for ksv list ready (spec says max time allowed is 5s) */ > - ret = __wait_for(read_ret = shim->read_ksv_ready(intel_dig_port, > - &ksv_ready), > - read_ret || ksv_ready, 5 * 1000 * 1000, 1000, > - 100 * 1000); > - if (ret) > - return ret; > - if (read_ret) > - return read_ret; > - if (!ksv_ready) > - return -ETIMEDOUT; > - > - return 0; > -} > - > static bool hdcp_key_loadable(struct drm_i915_private *dev_priv) > { > struct i915_power_domains *power_domains = &dev_priv->power_domains; > @@ -222,7 +201,7 @@ int intel_hdcp_auth_downstream(struct intel_digital_port *intel_dig_port, > > dev_priv = intel_dig_port->base.base.dev->dev_private; > > - ret = intel_hdcp_poll_ksv_fifo(intel_dig_port, shim); > + ret = shim->wait_for_ksv_ready(intel_dig_port); > if (ret) { > DRM_ERROR("KSV list failed to become ready (%d)\n", ret); > return ret; > @@ -476,7 +455,6 @@ static int intel_hdcp_auth(struct intel_digital_port *intel_dig_port, > { > struct drm_i915_private *dev_priv; > enum port port; > - unsigned long r0_prime_gen_start; > int ret, i, tries = 2; > union { > u32 reg[2]; > @@ -531,8 +509,6 @@ static int intel_hdcp_auth(struct intel_digital_port *intel_dig_port, > if (ret) > return ret; > > - r0_prime_gen_start = jiffies; > - > memset(&bksv, 0, sizeof(bksv)); > > /* HDCP spec states that we must retry the bksv if it is invalid */ > @@ -572,22 +548,11 @@ static int intel_hdcp_auth(struct intel_digital_port *intel_dig_port, > } > > /* > - * Wait for R0' to become available. The spec says minimum 100ms from > - * Aksv, but some monitors can take longer than this. So we are > - * combinely waiting for 300mSec just to be sure in case of HDMI. > * DP HDCP Spec mandates the two more reattempt to read R0, incase > * of R0 mismatch. > - * > - * On DP, there's an R0_READY bit available but no such bit > - * exists on HDMI. Since the upper-bound is the same, we'll just do > - * the stupid thing instead of polling on one and not the other. > */ > - > tries = 3; > for (i = 0; i < tries; i++) { > - wait_remaining_ms_from_jiffies(r0_prime_gen_start, > - 100 * (i + 1)); > - > ri.reg = 0; > ret = shim->read_ri_prime(intel_dig_port, ri.shim); > if (ret) > @@ -749,6 +714,8 @@ int intel_hdcp_init(struct intel_connector *connector, > mutex_init(&connector->hdcp_mutex); > INIT_DELAYED_WORK(&connector->hdcp_check_work, intel_hdcp_check_work); > INIT_WORK(&connector->hdcp_prop_work, intel_hdcp_prop_work); > + > + init_completion(&connector->cp_irq_recved); > return 0; > } > > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c > index f5d7bfb43006..532d13f91602 100644 > --- a/drivers/gpu/drm/i915/intel_hdmi.c > +++ b/drivers/gpu/drm/i915/intel_hdmi.c > @@ -1007,6 +1007,9 @@ int intel_hdmi_hdcp_read_ri_prime(struct intel_digital_port *intel_dig_port, > u8 *ri_prime) > { > int ret; > + > + wait_remaining_ms_from_jiffies(jiffies, 100); > + > ret = intel_hdmi_hdcp_read(intel_dig_port, DRM_HDCP_DDC_RI_PRIME, > ri_prime, DRM_HDCP_RI_LEN); > if (ret) > @@ -1027,6 +1030,29 @@ int intel_hdmi_hdcp_read_ksv_ready(struct intel_digital_port *intel_dig_port, > return ret; > } > *ksv_ready = val & DRM_HDCP_DDC_BCAPS_KSV_FIFO_READY; > + > + return 0; > +} > + > +static > +int intel_hdmi_hdcp_ksv_ready(struct intel_digital_port *intel_dig_port) > +{ > + int ret, read_ret; > + bool ksv_ready; > + > + /* Poll for ksv list ready (spec says max time allowed is 5s) */ > + ret = __wait_for(read_ret = > + intel_hdmi_hdcp_read_ksv_ready(intel_dig_port, > + &ksv_ready), > + read_ret || ksv_ready, 5 * 1000 * 1000, 1000, > + 100 * 1000); > + if (ret) > + return ret; > + if (read_ret) > + return read_ret; > + if (!ksv_ready) > + return -ETIMEDOUT; > + > return 0; > } > > @@ -1112,7 +1138,7 @@ static const struct intel_hdcp_shim intel_hdmi_hdcp_shim = { > .read_bstatus = intel_hdmi_hdcp_read_bstatus, > .repeater_present = intel_hdmi_hdcp_repeater_present, > .read_ri_prime = intel_hdmi_hdcp_read_ri_prime, > - .read_ksv_ready = intel_hdmi_hdcp_read_ksv_ready, > + .wait_for_ksv_ready = intel_hdmi_hdcp_ksv_ready, > .read_ksv_fifo = intel_hdmi_hdcp_read_ksv_fifo, > .read_v_prime_part = intel_hdmi_hdcp_read_v_prime_part, > .toggle_signalling = intel_hdmi_hdcp_toggle_signalling, > -- > 2.7.4 >
Quoting Ramalingam C (2018-02-26 17:12:39) > DP and HDMI HDCP specifications are varying with respect to > detecting the R0 and ksv_fifo availability. > > DP will produce CP_IRQ and set a bit for indicating the R0 and > FIFO_READY status. > > Whereas HDMI will set a bit for FIFO_READY but there is no bit > indication for R0 ready. And also polling of READY bit is needed for > HDMI as ther is no CP_IRQ. > > So Fielding the CP_IRQ for DP and the polling the READY bit for a > period and blindly waiting for a fixed time for R0 incase of HDMI are > moved into corresponding hdcp_shim. > > Signed-off-by: Ramalingam C <ramalingam.c@intel.com> > --- > +static int intel_dp_hdcp_wait_for_cp_irq(struct completion *cp_irq_recved, > + int timeout) > +{ > + long ret; > + > + if (completion_done(cp_irq_recved)) > + reinit_completion(cp_irq_recved); > + > + ret = wait_for_completion_interruptible_timeout(cp_irq_recved, > + msecs_to_jiffies( > + timeout)); > + reinit_completion(cp_irq_recved); This is not thread-safe. The trick is to use a waitqueue to interrupt the sleeps inside the wait loops to complete the polling quicker. (As stated, you can't escape the polling, and you always need to check the IRQ was for the event you expected anyway.) -Chris
On Monday 26 February 2018 11:31 PM, Sean Paul wrote: > On Mon, Feb 26, 2018 at 10:42:39PM +0530, Ramalingam C wrote: >> DP and HDMI HDCP specifications are varying with respect to >> detecting the R0 and ksv_fifo availability. >> >> DP will produce CP_IRQ and set a bit for indicating the R0 and >> FIFO_READY status. > I'm not sure what the benefit is? Keeping things common was a deliberate > decision on my part to keep complexity down and increase the amount of common > code. I am proposing this expecting that this will address the DP compliance failure incase of regular repeater tests(attempt to read wrong FIFO size, due to wrong device count). Possibly a corner case!?. And since anyway we have the shim for handling the DP and HDMI specific code, i feel this will do only better but harm. --Ram > > Sean > >> Whereas HDMI will set a bit for FIFO_READY but there is no bit >> indication for R0 ready. And also polling of READY bit is needed for >> HDMI as ther is no CP_IRQ. >> >> So Fielding the CP_IRQ for DP and the polling the READY bit for a >> period and blindly waiting for a fixed time for R0 incase of HDMI are >> moved into corresponding hdcp_shim. >> >> Signed-off-by: Ramalingam C <ramalingam.c@intel.com> >> --- >> drivers/gpu/drm/i915/intel_dp.c | 70 +++++++++++++++++++++++++++++++++++---- >> drivers/gpu/drm/i915/intel_drv.h | 6 ++-- >> drivers/gpu/drm/i915/intel_hdcp.c | 39 ++-------------------- >> drivers/gpu/drm/i915/intel_hdmi.c | 28 +++++++++++++++- >> 4 files changed, 98 insertions(+), 45 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c >> index 2c3eb90b9499..afe310a91d3c 100644 >> --- a/drivers/gpu/drm/i915/intel_dp.c >> +++ b/drivers/gpu/drm/i915/intel_dp.c >> @@ -4436,6 +4436,7 @@ static bool >> intel_dp_short_pulse(struct intel_dp *intel_dp) >> { >> struct drm_i915_private *dev_priv = to_i915(intel_dp_to_dev(intel_dp)); >> + struct intel_connector *connector = intel_dp->attached_connector; >> u8 sink_irq_vector = 0; >> u8 old_sink_count = intel_dp->sink_count; >> bool ret; >> @@ -4470,8 +4471,11 @@ intel_dp_short_pulse(struct intel_dp *intel_dp) >> >> if (sink_irq_vector & DP_AUTOMATED_TEST_REQUEST) >> intel_dp_handle_test_request(intel_dp); >> - if (sink_irq_vector & (DP_CP_IRQ | DP_SINK_SPECIFIC_IRQ)) >> - DRM_DEBUG_DRIVER("CP or sink specific irq unhandled\n"); >> + if (sink_irq_vector & DP_CP_IRQ) >> + if (connector->hdcp_shim) >> + complete_all(&connector->cp_irq_recved); >> + if (sink_irq_vector & DP_SINK_SPECIFIC_IRQ) >> + DRM_DEBUG_DRIVER("Sink specific irq unhandled\n"); >> } >> >> intel_dp_check_link_status(intel_dp); >> @@ -5083,6 +5087,25 @@ void intel_dp_encoder_suspend(struct intel_encoder *intel_encoder) >> pps_unlock(intel_dp); >> } >> >> +static int intel_dp_hdcp_wait_for_cp_irq(struct completion *cp_irq_recved, >> + int timeout) >> +{ >> + long ret; >> + >> + if (completion_done(cp_irq_recved)) >> + reinit_completion(cp_irq_recved); >> + >> + ret = wait_for_completion_interruptible_timeout(cp_irq_recved, >> + msecs_to_jiffies( >> + timeout)); >> + reinit_completion(cp_irq_recved); >> + if (ret < 0) >> + return (int)ret; >> + else if (!ret) >> + return -ETIMEDOUT; >> + return 0; >> +} >> + >> static >> int intel_dp_hdcp_write_an_aksv(struct intel_digital_port *intel_dig_port, >> u8 *an) >> @@ -5188,11 +5211,38 @@ int intel_dp_hdcp_repeater_present(struct intel_digital_port *intel_dig_port, >> return 0; >> } >> >> +static int intel_dp_hdcp_wait_for_r0(struct intel_digital_port *intel_dig_port) >> +{ >> + struct intel_dp *dp = &intel_dig_port->dp; >> + struct intel_connector *connector = dp->attached_connector; >> + int ret; >> + u8 bstatus; >> + >> + intel_dp_hdcp_wait_for_cp_irq(&connector->cp_irq_recved, 100); >> + >> + ret = drm_dp_dpcd_read(&intel_dig_port->dp.aux, DP_AUX_HDCP_BSTATUS, >> + &bstatus, 1); >> + if (ret != 1) { >> + DRM_ERROR("Read bstatus from DP/AUX failed (%d)\n", ret); >> + return ret >= 0 ? -EIO : ret; >> + } >> + >> + if (!(bstatus & DP_BSTATUS_R0_PRIME_READY)) >> + return -ETIMEDOUT; >> + >> + return 0; >> +} >> + >> static >> int intel_dp_hdcp_read_ri_prime(struct intel_digital_port *intel_dig_port, >> u8 *ri_prime) >> { >> ssize_t ret; >> + >> + ret = intel_dp_hdcp_wait_for_r0(intel_dig_port); >> + if (!ret) >> + return ret; >> + >> ret = drm_dp_dpcd_read(&intel_dig_port->dp.aux, DP_AUX_HDCP_RI_PRIME, >> ri_prime, DRM_HDCP_RI_LEN); >> if (ret != DRM_HDCP_RI_LEN) { >> @@ -5203,18 +5253,26 @@ int intel_dp_hdcp_read_ri_prime(struct intel_digital_port *intel_dig_port, >> } >> >> static >> -int intel_dp_hdcp_read_ksv_ready(struct intel_digital_port *intel_dig_port, >> - bool *ksv_ready) >> +int intel_dp_hdcp_ksv_ready(struct intel_digital_port *intel_dig_port) >> { >> + struct intel_dp *dp = &intel_dig_port->dp; >> + struct intel_connector *connector = dp->attached_connector; >> ssize_t ret; >> u8 bstatus; >> + >> + intel_dp_hdcp_wait_for_cp_irq(&connector->cp_irq_recved, >> + 5 * 1000 * 1000); >> + >> ret = drm_dp_dpcd_read(&intel_dig_port->dp.aux, DP_AUX_HDCP_BSTATUS, >> &bstatus, 1); >> if (ret != 1) { >> DRM_ERROR("Read bstatus from DP/AUX failed (%zd)\n", ret); >> return ret >= 0 ? -EIO : ret; >> } >> - *ksv_ready = bstatus & DP_BSTATUS_READY; >> + >> + if (!(bstatus & DP_BSTATUS_READY)) >> + return -ETIMEDOUT; >> + >> return 0; >> } >> >> @@ -5305,7 +5363,7 @@ static const struct intel_hdcp_shim intel_dp_hdcp_shim = { >> .read_bstatus = intel_dp_hdcp_read_bstatus, >> .repeater_present = intel_dp_hdcp_repeater_present, >> .read_ri_prime = intel_dp_hdcp_read_ri_prime, >> - .read_ksv_ready = intel_dp_hdcp_read_ksv_ready, >> + .wait_for_ksv_ready = intel_dp_hdcp_ksv_ready, >> .read_ksv_fifo = intel_dp_hdcp_read_ksv_fifo, >> .read_v_prime_part = intel_dp_hdcp_read_v_prime_part, >> .toggle_signalling = intel_dp_hdcp_toggle_signalling, >> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h >> index 8f38e584d375..ab975107c978 100644 >> --- a/drivers/gpu/drm/i915/intel_drv.h >> +++ b/drivers/gpu/drm/i915/intel_drv.h >> @@ -353,8 +353,7 @@ struct intel_hdcp_shim { >> int (*read_ri_prime)(struct intel_digital_port *intel_dig_port, u8 *ri); >> >> /* Determines if the receiver's KSV FIFO is ready for consumption */ >> - int (*read_ksv_ready)(struct intel_digital_port *intel_dig_port, >> - bool *ksv_ready); >> + int (*wait_for_ksv_ready)(struct intel_digital_port *intel_dig_port); >> >> /* Reads the ksv fifo for num_downstream devices */ >> int (*read_ksv_fifo)(struct intel_digital_port *intel_dig_port, >> @@ -413,6 +412,9 @@ struct intel_connector { >> uint64_t hdcp_value; /* protected by hdcp_mutex */ >> struct delayed_work hdcp_check_work; >> struct work_struct hdcp_prop_work; >> + >> + /* To indicate the assertion of CP_IRQ */ >> + struct completion cp_irq_recved; >> }; >> >> struct intel_digital_connector_state { >> diff --git a/drivers/gpu/drm/i915/intel_hdcp.c b/drivers/gpu/drm/i915/intel_hdcp.c >> index 14be14a45e5a..a077e1333886 100644 >> --- a/drivers/gpu/drm/i915/intel_hdcp.c >> +++ b/drivers/gpu/drm/i915/intel_hdcp.c >> @@ -57,27 +57,6 @@ static bool wait_for_hdcp_port(struct intel_connector *connector) >> return true; >> } >> >> -static int intel_hdcp_poll_ksv_fifo(struct intel_digital_port *intel_dig_port, >> - const struct intel_hdcp_shim *shim) >> -{ >> - int ret, read_ret; >> - bool ksv_ready; >> - >> - /* Poll for ksv list ready (spec says max time allowed is 5s) */ >> - ret = __wait_for(read_ret = shim->read_ksv_ready(intel_dig_port, >> - &ksv_ready), >> - read_ret || ksv_ready, 5 * 1000 * 1000, 1000, >> - 100 * 1000); >> - if (ret) >> - return ret; >> - if (read_ret) >> - return read_ret; >> - if (!ksv_ready) >> - return -ETIMEDOUT; >> - >> - return 0; >> -} >> - >> static bool hdcp_key_loadable(struct drm_i915_private *dev_priv) >> { >> struct i915_power_domains *power_domains = &dev_priv->power_domains; >> @@ -222,7 +201,7 @@ int intel_hdcp_auth_downstream(struct intel_digital_port *intel_dig_port, >> >> dev_priv = intel_dig_port->base.base.dev->dev_private; >> >> - ret = intel_hdcp_poll_ksv_fifo(intel_dig_port, shim); >> + ret = shim->wait_for_ksv_ready(intel_dig_port); >> if (ret) { >> DRM_ERROR("KSV list failed to become ready (%d)\n", ret); >> return ret; >> @@ -476,7 +455,6 @@ static int intel_hdcp_auth(struct intel_digital_port *intel_dig_port, >> { >> struct drm_i915_private *dev_priv; >> enum port port; >> - unsigned long r0_prime_gen_start; >> int ret, i, tries = 2; >> union { >> u32 reg[2]; >> @@ -531,8 +509,6 @@ static int intel_hdcp_auth(struct intel_digital_port *intel_dig_port, >> if (ret) >> return ret; >> >> - r0_prime_gen_start = jiffies; >> - >> memset(&bksv, 0, sizeof(bksv)); >> >> /* HDCP spec states that we must retry the bksv if it is invalid */ >> @@ -572,22 +548,11 @@ static int intel_hdcp_auth(struct intel_digital_port *intel_dig_port, >> } >> >> /* >> - * Wait for R0' to become available. The spec says minimum 100ms from >> - * Aksv, but some monitors can take longer than this. So we are >> - * combinely waiting for 300mSec just to be sure in case of HDMI. >> * DP HDCP Spec mandates the two more reattempt to read R0, incase >> * of R0 mismatch. >> - * >> - * On DP, there's an R0_READY bit available but no such bit >> - * exists on HDMI. Since the upper-bound is the same, we'll just do >> - * the stupid thing instead of polling on one and not the other. >> */ >> - >> tries = 3; >> for (i = 0; i < tries; i++) { >> - wait_remaining_ms_from_jiffies(r0_prime_gen_start, >> - 100 * (i + 1)); >> - >> ri.reg = 0; >> ret = shim->read_ri_prime(intel_dig_port, ri.shim); >> if (ret) >> @@ -749,6 +714,8 @@ int intel_hdcp_init(struct intel_connector *connector, >> mutex_init(&connector->hdcp_mutex); >> INIT_DELAYED_WORK(&connector->hdcp_check_work, intel_hdcp_check_work); >> INIT_WORK(&connector->hdcp_prop_work, intel_hdcp_prop_work); >> + >> + init_completion(&connector->cp_irq_recved); >> return 0; >> } >> >> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c >> index f5d7bfb43006..532d13f91602 100644 >> --- a/drivers/gpu/drm/i915/intel_hdmi.c >> +++ b/drivers/gpu/drm/i915/intel_hdmi.c >> @@ -1007,6 +1007,9 @@ int intel_hdmi_hdcp_read_ri_prime(struct intel_digital_port *intel_dig_port, >> u8 *ri_prime) >> { >> int ret; >> + >> + wait_remaining_ms_from_jiffies(jiffies, 100); >> + >> ret = intel_hdmi_hdcp_read(intel_dig_port, DRM_HDCP_DDC_RI_PRIME, >> ri_prime, DRM_HDCP_RI_LEN); >> if (ret) >> @@ -1027,6 +1030,29 @@ int intel_hdmi_hdcp_read_ksv_ready(struct intel_digital_port *intel_dig_port, >> return ret; >> } >> *ksv_ready = val & DRM_HDCP_DDC_BCAPS_KSV_FIFO_READY; >> + >> + return 0; >> +} >> + >> +static >> +int intel_hdmi_hdcp_ksv_ready(struct intel_digital_port *intel_dig_port) >> +{ >> + int ret, read_ret; >> + bool ksv_ready; >> + >> + /* Poll for ksv list ready (spec says max time allowed is 5s) */ >> + ret = __wait_for(read_ret = >> + intel_hdmi_hdcp_read_ksv_ready(intel_dig_port, >> + &ksv_ready), >> + read_ret || ksv_ready, 5 * 1000 * 1000, 1000, >> + 100 * 1000); >> + if (ret) >> + return ret; >> + if (read_ret) >> + return read_ret; >> + if (!ksv_ready) >> + return -ETIMEDOUT; >> + >> return 0; >> } >> >> @@ -1112,7 +1138,7 @@ static const struct intel_hdcp_shim intel_hdmi_hdcp_shim = { >> .read_bstatus = intel_hdmi_hdcp_read_bstatus, >> .repeater_present = intel_hdmi_hdcp_repeater_present, >> .read_ri_prime = intel_hdmi_hdcp_read_ri_prime, >> - .read_ksv_ready = intel_hdmi_hdcp_read_ksv_ready, >> + .wait_for_ksv_ready = intel_hdmi_hdcp_ksv_ready, >> .read_ksv_fifo = intel_hdmi_hdcp_read_ksv_fifo, >> .read_v_prime_part = intel_hdmi_hdcp_read_v_prime_part, >> .toggle_signalling = intel_hdmi_hdcp_toggle_signalling, >> -- >> 2.7.4 >>
On Tuesday 27 February 2018 04:20 AM, Chris Wilson wrote: > Quoting Ramalingam C (2018-02-26 17:12:39) >> DP and HDMI HDCP specifications are varying with respect to >> detecting the R0 and ksv_fifo availability. >> >> DP will produce CP_IRQ and set a bit for indicating the R0 and >> FIFO_READY status. >> >> Whereas HDMI will set a bit for FIFO_READY but there is no bit >> indication for R0 ready. And also polling of READY bit is needed for >> HDMI as ther is no CP_IRQ. >> >> So Fielding the CP_IRQ for DP and the polling the READY bit for a >> period and blindly waiting for a fixed time for R0 incase of HDMI are >> moved into corresponding hdcp_shim. >> >> Signed-off-by: Ramalingam C <ramalingam.c@intel.com> >> --- >> +static int intel_dp_hdcp_wait_for_cp_irq(struct completion *cp_irq_recved, >> + int timeout) >> +{ >> + long ret; >> + >> + if (completion_done(cp_irq_recved)) >> + reinit_completion(cp_irq_recved); >> + >> + ret = wait_for_completion_interruptible_timeout(cp_irq_recved, >> + msecs_to_jiffies( >> + timeout)); >> + reinit_completion(cp_irq_recved); > This is not thread-safe. > > The trick is to use a waitqueue to interrupt the sleeps inside the wait > loops to complete the polling quicker. (As stated, you can't escape the > polling, and you always need to check the IRQ was for the event you > expected anyway.) > -Chris Chris, I think I am lost here. Could you please help me understand what are we missing here? Completion also uses the wait_queue and each thread that want to wait for a event waits on it. And on IRQ arrival through complete_all we are marking the completion->done and waking up all the thread sleeping at completion wait_queue. Are you suggesting wait_event_timeout as we have in intel_dp_aux_wait_done? Thanks, --Ram
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 2c3eb90b9499..afe310a91d3c 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -4436,6 +4436,7 @@ static bool intel_dp_short_pulse(struct intel_dp *intel_dp) { struct drm_i915_private *dev_priv = to_i915(intel_dp_to_dev(intel_dp)); + struct intel_connector *connector = intel_dp->attached_connector; u8 sink_irq_vector = 0; u8 old_sink_count = intel_dp->sink_count; bool ret; @@ -4470,8 +4471,11 @@ intel_dp_short_pulse(struct intel_dp *intel_dp) if (sink_irq_vector & DP_AUTOMATED_TEST_REQUEST) intel_dp_handle_test_request(intel_dp); - if (sink_irq_vector & (DP_CP_IRQ | DP_SINK_SPECIFIC_IRQ)) - DRM_DEBUG_DRIVER("CP or sink specific irq unhandled\n"); + if (sink_irq_vector & DP_CP_IRQ) + if (connector->hdcp_shim) + complete_all(&connector->cp_irq_recved); + if (sink_irq_vector & DP_SINK_SPECIFIC_IRQ) + DRM_DEBUG_DRIVER("Sink specific irq unhandled\n"); } intel_dp_check_link_status(intel_dp); @@ -5083,6 +5087,25 @@ void intel_dp_encoder_suspend(struct intel_encoder *intel_encoder) pps_unlock(intel_dp); } +static int intel_dp_hdcp_wait_for_cp_irq(struct completion *cp_irq_recved, + int timeout) +{ + long ret; + + if (completion_done(cp_irq_recved)) + reinit_completion(cp_irq_recved); + + ret = wait_for_completion_interruptible_timeout(cp_irq_recved, + msecs_to_jiffies( + timeout)); + reinit_completion(cp_irq_recved); + if (ret < 0) + return (int)ret; + else if (!ret) + return -ETIMEDOUT; + return 0; +} + static int intel_dp_hdcp_write_an_aksv(struct intel_digital_port *intel_dig_port, u8 *an) @@ -5188,11 +5211,38 @@ int intel_dp_hdcp_repeater_present(struct intel_digital_port *intel_dig_port, return 0; } +static int intel_dp_hdcp_wait_for_r0(struct intel_digital_port *intel_dig_port) +{ + struct intel_dp *dp = &intel_dig_port->dp; + struct intel_connector *connector = dp->attached_connector; + int ret; + u8 bstatus; + + intel_dp_hdcp_wait_for_cp_irq(&connector->cp_irq_recved, 100); + + ret = drm_dp_dpcd_read(&intel_dig_port->dp.aux, DP_AUX_HDCP_BSTATUS, + &bstatus, 1); + if (ret != 1) { + DRM_ERROR("Read bstatus from DP/AUX failed (%d)\n", ret); + return ret >= 0 ? -EIO : ret; + } + + if (!(bstatus & DP_BSTATUS_R0_PRIME_READY)) + return -ETIMEDOUT; + + return 0; +} + static int intel_dp_hdcp_read_ri_prime(struct intel_digital_port *intel_dig_port, u8 *ri_prime) { ssize_t ret; + + ret = intel_dp_hdcp_wait_for_r0(intel_dig_port); + if (!ret) + return ret; + ret = drm_dp_dpcd_read(&intel_dig_port->dp.aux, DP_AUX_HDCP_RI_PRIME, ri_prime, DRM_HDCP_RI_LEN); if (ret != DRM_HDCP_RI_LEN) { @@ -5203,18 +5253,26 @@ int intel_dp_hdcp_read_ri_prime(struct intel_digital_port *intel_dig_port, } static -int intel_dp_hdcp_read_ksv_ready(struct intel_digital_port *intel_dig_port, - bool *ksv_ready) +int intel_dp_hdcp_ksv_ready(struct intel_digital_port *intel_dig_port) { + struct intel_dp *dp = &intel_dig_port->dp; + struct intel_connector *connector = dp->attached_connector; ssize_t ret; u8 bstatus; + + intel_dp_hdcp_wait_for_cp_irq(&connector->cp_irq_recved, + 5 * 1000 * 1000); + ret = drm_dp_dpcd_read(&intel_dig_port->dp.aux, DP_AUX_HDCP_BSTATUS, &bstatus, 1); if (ret != 1) { DRM_ERROR("Read bstatus from DP/AUX failed (%zd)\n", ret); return ret >= 0 ? -EIO : ret; } - *ksv_ready = bstatus & DP_BSTATUS_READY; + + if (!(bstatus & DP_BSTATUS_READY)) + return -ETIMEDOUT; + return 0; } @@ -5305,7 +5363,7 @@ static const struct intel_hdcp_shim intel_dp_hdcp_shim = { .read_bstatus = intel_dp_hdcp_read_bstatus, .repeater_present = intel_dp_hdcp_repeater_present, .read_ri_prime = intel_dp_hdcp_read_ri_prime, - .read_ksv_ready = intel_dp_hdcp_read_ksv_ready, + .wait_for_ksv_ready = intel_dp_hdcp_ksv_ready, .read_ksv_fifo = intel_dp_hdcp_read_ksv_fifo, .read_v_prime_part = intel_dp_hdcp_read_v_prime_part, .toggle_signalling = intel_dp_hdcp_toggle_signalling, diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 8f38e584d375..ab975107c978 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -353,8 +353,7 @@ struct intel_hdcp_shim { int (*read_ri_prime)(struct intel_digital_port *intel_dig_port, u8 *ri); /* Determines if the receiver's KSV FIFO is ready for consumption */ - int (*read_ksv_ready)(struct intel_digital_port *intel_dig_port, - bool *ksv_ready); + int (*wait_for_ksv_ready)(struct intel_digital_port *intel_dig_port); /* Reads the ksv fifo for num_downstream devices */ int (*read_ksv_fifo)(struct intel_digital_port *intel_dig_port, @@ -413,6 +412,9 @@ struct intel_connector { uint64_t hdcp_value; /* protected by hdcp_mutex */ struct delayed_work hdcp_check_work; struct work_struct hdcp_prop_work; + + /* To indicate the assertion of CP_IRQ */ + struct completion cp_irq_recved; }; struct intel_digital_connector_state { diff --git a/drivers/gpu/drm/i915/intel_hdcp.c b/drivers/gpu/drm/i915/intel_hdcp.c index 14be14a45e5a..a077e1333886 100644 --- a/drivers/gpu/drm/i915/intel_hdcp.c +++ b/drivers/gpu/drm/i915/intel_hdcp.c @@ -57,27 +57,6 @@ static bool wait_for_hdcp_port(struct intel_connector *connector) return true; } -static int intel_hdcp_poll_ksv_fifo(struct intel_digital_port *intel_dig_port, - const struct intel_hdcp_shim *shim) -{ - int ret, read_ret; - bool ksv_ready; - - /* Poll for ksv list ready (spec says max time allowed is 5s) */ - ret = __wait_for(read_ret = shim->read_ksv_ready(intel_dig_port, - &ksv_ready), - read_ret || ksv_ready, 5 * 1000 * 1000, 1000, - 100 * 1000); - if (ret) - return ret; - if (read_ret) - return read_ret; - if (!ksv_ready) - return -ETIMEDOUT; - - return 0; -} - static bool hdcp_key_loadable(struct drm_i915_private *dev_priv) { struct i915_power_domains *power_domains = &dev_priv->power_domains; @@ -222,7 +201,7 @@ int intel_hdcp_auth_downstream(struct intel_digital_port *intel_dig_port, dev_priv = intel_dig_port->base.base.dev->dev_private; - ret = intel_hdcp_poll_ksv_fifo(intel_dig_port, shim); + ret = shim->wait_for_ksv_ready(intel_dig_port); if (ret) { DRM_ERROR("KSV list failed to become ready (%d)\n", ret); return ret; @@ -476,7 +455,6 @@ static int intel_hdcp_auth(struct intel_digital_port *intel_dig_port, { struct drm_i915_private *dev_priv; enum port port; - unsigned long r0_prime_gen_start; int ret, i, tries = 2; union { u32 reg[2]; @@ -531,8 +509,6 @@ static int intel_hdcp_auth(struct intel_digital_port *intel_dig_port, if (ret) return ret; - r0_prime_gen_start = jiffies; - memset(&bksv, 0, sizeof(bksv)); /* HDCP spec states that we must retry the bksv if it is invalid */ @@ -572,22 +548,11 @@ static int intel_hdcp_auth(struct intel_digital_port *intel_dig_port, } /* - * Wait for R0' to become available. The spec says minimum 100ms from - * Aksv, but some monitors can take longer than this. So we are - * combinely waiting for 300mSec just to be sure in case of HDMI. * DP HDCP Spec mandates the two more reattempt to read R0, incase * of R0 mismatch. - * - * On DP, there's an R0_READY bit available but no such bit - * exists on HDMI. Since the upper-bound is the same, we'll just do - * the stupid thing instead of polling on one and not the other. */ - tries = 3; for (i = 0; i < tries; i++) { - wait_remaining_ms_from_jiffies(r0_prime_gen_start, - 100 * (i + 1)); - ri.reg = 0; ret = shim->read_ri_prime(intel_dig_port, ri.shim); if (ret) @@ -749,6 +714,8 @@ int intel_hdcp_init(struct intel_connector *connector, mutex_init(&connector->hdcp_mutex); INIT_DELAYED_WORK(&connector->hdcp_check_work, intel_hdcp_check_work); INIT_WORK(&connector->hdcp_prop_work, intel_hdcp_prop_work); + + init_completion(&connector->cp_irq_recved); return 0; } diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c index f5d7bfb43006..532d13f91602 100644 --- a/drivers/gpu/drm/i915/intel_hdmi.c +++ b/drivers/gpu/drm/i915/intel_hdmi.c @@ -1007,6 +1007,9 @@ int intel_hdmi_hdcp_read_ri_prime(struct intel_digital_port *intel_dig_port, u8 *ri_prime) { int ret; + + wait_remaining_ms_from_jiffies(jiffies, 100); + ret = intel_hdmi_hdcp_read(intel_dig_port, DRM_HDCP_DDC_RI_PRIME, ri_prime, DRM_HDCP_RI_LEN); if (ret) @@ -1027,6 +1030,29 @@ int intel_hdmi_hdcp_read_ksv_ready(struct intel_digital_port *intel_dig_port, return ret; } *ksv_ready = val & DRM_HDCP_DDC_BCAPS_KSV_FIFO_READY; + + return 0; +} + +static +int intel_hdmi_hdcp_ksv_ready(struct intel_digital_port *intel_dig_port) +{ + int ret, read_ret; + bool ksv_ready; + + /* Poll for ksv list ready (spec says max time allowed is 5s) */ + ret = __wait_for(read_ret = + intel_hdmi_hdcp_read_ksv_ready(intel_dig_port, + &ksv_ready), + read_ret || ksv_ready, 5 * 1000 * 1000, 1000, + 100 * 1000); + if (ret) + return ret; + if (read_ret) + return read_ret; + if (!ksv_ready) + return -ETIMEDOUT; + return 0; } @@ -1112,7 +1138,7 @@ static const struct intel_hdcp_shim intel_hdmi_hdcp_shim = { .read_bstatus = intel_hdmi_hdcp_read_bstatus, .repeater_present = intel_hdmi_hdcp_repeater_present, .read_ri_prime = intel_hdmi_hdcp_read_ri_prime, - .read_ksv_ready = intel_hdmi_hdcp_read_ksv_ready, + .wait_for_ksv_ready = intel_hdmi_hdcp_ksv_ready, .read_ksv_fifo = intel_hdmi_hdcp_read_ksv_fifo, .read_v_prime_part = intel_hdmi_hdcp_read_v_prime_part, .toggle_signalling = intel_hdmi_hdcp_toggle_signalling,
DP and HDMI HDCP specifications are varying with respect to detecting the R0 and ksv_fifo availability. DP will produce CP_IRQ and set a bit for indicating the R0 and FIFO_READY status. Whereas HDMI will set a bit for FIFO_READY but there is no bit indication for R0 ready. And also polling of READY bit is needed for HDMI as ther is no CP_IRQ. So Fielding the CP_IRQ for DP and the polling the READY bit for a period and blindly waiting for a fixed time for R0 incase of HDMI are moved into corresponding hdcp_shim. Signed-off-by: Ramalingam C <ramalingam.c@intel.com> --- drivers/gpu/drm/i915/intel_dp.c | 70 +++++++++++++++++++++++++++++++++++---- drivers/gpu/drm/i915/intel_drv.h | 6 ++-- drivers/gpu/drm/i915/intel_hdcp.c | 39 ++-------------------- drivers/gpu/drm/i915/intel_hdmi.c | 28 +++++++++++++++- 4 files changed, 98 insertions(+), 45 deletions(-)