| Message ID | 20180418224311.16577-7-jose.souza@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, 2018-04-18 at 15:43 -0700, José Roberto de Souza wrote: > This reduces the spaghetti that intel_dp_aux_xfer(). > > Moved doing less changes possible here, improvements to the new > function in further patch. > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > --- > > New patch in this series. > > drivers/gpu/drm/i915/intel_dp.c | 52 +++++++++++++++++++++------------ > 1 file changed, 34 insertions(+), 18 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index 701963a192ee..a11465c62950 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -1062,6 +1062,38 @@ static uint32_t skl_get_aux_send_ctl(struct intel_dp *intel_dp, > DP_AUX_CH_CTL_SYNC_PULSE_SKL(32); > } > > +static bool intel_dp_aux_is_busy(struct intel_dp *intel_dp) > +{ > + struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); > + struct drm_i915_private *dev_priv = > + to_i915(intel_dig_port->base.base.dev); > + i915_reg_t ch_ctl; > + uint32_t status; > + int try; > + > + ch_ctl = intel_dp->aux_ch_ctl_reg(intel_dp); > + > + for (try = 0; try < 3; try++) { > + status = I915_READ_NOTRACE(ch_ctl); > + if ((status & DP_AUX_CH_CTL_SEND_BUSY) == 0) > + break; Did you mean to return false here? Anyway, this code here looks very similar to intel_dp_aux_wait_done(). Might be worth checking if it can be reused. > + msleep(1); > + } > + > + if (try == 3) { > + static u32 last_status = -1; > + const u32 status = I915_READ(ch_ctl); > + > + if (status != last_status) { > + WARN(1, "dp_aux_ch not started status 0x%08x\n", > + status); > + last_status = status; > + } > + } > + > + return true; > +} > + > static int > intel_dp_aux_xfer(struct intel_dp *intel_dp, > const uint8_t *send, int send_bytes, > @@ -1074,7 +1106,7 @@ intel_dp_aux_xfer(struct intel_dp *intel_dp, > i915_reg_t ch_ctl, ch_data[5]; > uint32_t aux_clock_divider; > int i, ret, recv_bytes; > - uint32_t status; > + uint32_t status = 0; > int try, clock = 0; > bool has_aux_irq = HAS_AUX_IRQ(dev_priv); > bool vdd; > @@ -1102,23 +1134,7 @@ intel_dp_aux_xfer(struct intel_dp *intel_dp, > intel_dp_check_edp(intel_dp); > > /* Try to wait for any previous AUX channel activity */ > - for (try = 0; try < 3; try++) { > - status = I915_READ_NOTRACE(ch_ctl); > - if ((status & DP_AUX_CH_CTL_SEND_BUSY) == 0) > - break; > - msleep(1); > - } > - > - if (try == 3) { > - static u32 last_status = -1; > - const u32 status = I915_READ(ch_ctl); > - > - if (status != last_status) { > - WARN(1, "dp_aux_ch not started status 0x%08x\n", > - status); > - last_status = status; > - } > - > + if (intel_dp_aux_is_busy(intel_dp)) { > ret = -EBUSY; > goto out; > }
On Thu, 2018-04-26 at 15:51 -0700, Dhinakaran Pandiyan wrote: > > > On Wed, 2018-04-18 at 15:43 -0700, José Roberto de Souza wrote: > > This reduces the spaghetti that intel_dp_aux_xfer(). > > > > Moved doing less changes possible here, improvements to the new > > function in further patch. > > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > --- > > > > New patch in this series. > > > > drivers/gpu/drm/i915/intel_dp.c | 52 +++++++++++++++++++++------ > > ------ > > 1 file changed, 34 insertions(+), 18 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c > > b/drivers/gpu/drm/i915/intel_dp.c > > index 701963a192ee..a11465c62950 100644 > > --- a/drivers/gpu/drm/i915/intel_dp.c > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > @@ -1062,6 +1062,38 @@ static uint32_t skl_get_aux_send_ctl(struct > > intel_dp *intel_dp, > > DP_AUX_CH_CTL_SYNC_PULSE_SKL(32); > > } > > > > +static bool intel_dp_aux_is_busy(struct intel_dp *intel_dp) > > +{ > > + struct intel_digital_port *intel_dig_port = > > dp_to_dig_port(intel_dp); > > + struct drm_i915_private *dev_priv = > > + to_i915(intel_dig_port->base.base.dev); > > + i915_reg_t ch_ctl; > > + uint32_t status; > > + int try; > > + > > + ch_ctl = intel_dp->aux_ch_ctl_reg(intel_dp); > > + > > + for (try = 0; try < 3; try++) { > > + status = I915_READ_NOTRACE(ch_ctl); > > + if ((status & DP_AUX_CH_CTL_SEND_BUSY) == 0) > > + break; > > Did you mean to return false here? Oh thanks, I just fixed this in the next patch. > > Anyway, this code here looks very similar to > intel_dp_aux_wait_done(). > Might be worth checking if it can be reused. I'm not sure that hardware will send a interruption when it finish a aux ch transaction that started by it self, that is why I'm going safe here and just keep what is working for now. > > > + msleep(1); > > + } > > + > > + if (try == 3) { > > + static u32 last_status = -1; > > + const u32 status = I915_READ(ch_ctl); > > + > > + if (status != last_status) { > > + WARN(1, "dp_aux_ch not started status > > 0x%08x\n", > > + status); > > + last_status = status; > > + } > > + } > > + > > + return true; > > +} > > + > > static int > > intel_dp_aux_xfer(struct intel_dp *intel_dp, > > const uint8_t *send, int send_bytes, > > @@ -1074,7 +1106,7 @@ intel_dp_aux_xfer(struct intel_dp *intel_dp, > > i915_reg_t ch_ctl, ch_data[5]; > > uint32_t aux_clock_divider; > > int i, ret, recv_bytes; > > - uint32_t status; > > + uint32_t status = 0; > > int try, clock = 0; > > bool has_aux_irq = HAS_AUX_IRQ(dev_priv); > > bool vdd; > > @@ -1102,23 +1134,7 @@ intel_dp_aux_xfer(struct intel_dp *intel_dp, > > intel_dp_check_edp(intel_dp); > > > > /* Try to wait for any previous AUX channel activity */ > > - for (try = 0; try < 3; try++) { > > - status = I915_READ_NOTRACE(ch_ctl); > > - if ((status & DP_AUX_CH_CTL_SEND_BUSY) == 0) > > - break; > > - msleep(1); > > - } > > - > > - if (try == 3) { > > - static u32 last_status = -1; > > - const u32 status = I915_READ(ch_ctl); > > - > > - if (status != last_status) { > > - WARN(1, "dp_aux_ch not started status > > 0x%08x\n", > > - status); > > - last_status = status; > > - } > > - > > + if (intel_dp_aux_is_busy(intel_dp)) { > > ret = -EBUSY; > > goto out; > > } > >
On Mon, 2018-04-30 at 23:39 +0000, Souza, Jose wrote: > On Thu, 2018-04-26 at 15:51 -0700, Dhinakaran Pandiyan wrote: > > > > > > > > On Wed, 2018-04-18 at 15:43 -0700, José Roberto de Souza wrote: > > > > > > This reduces the spaghetti that intel_dp_aux_xfer(). > > > > > > Moved doing less changes possible here, improvements to the new > > > function in further patch. > > > > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > > --- > > > > > > New patch in this series. > > > > > > drivers/gpu/drm/i915/intel_dp.c | 52 +++++++++++++++++++++------ > > > ------ > > > 1 file changed, 34 insertions(+), 18 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c > > > b/drivers/gpu/drm/i915/intel_dp.c > > > index 701963a192ee..a11465c62950 100644 > > > --- a/drivers/gpu/drm/i915/intel_dp.c > > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > > @@ -1062,6 +1062,38 @@ static uint32_t > > > skl_get_aux_send_ctl(struct > > > intel_dp *intel_dp, > > > DP_AUX_CH_CTL_SYNC_PULSE_SKL(32); > > > } > > > > > > +static bool intel_dp_aux_is_busy(struct intel_dp *intel_dp) > > > +{ > > > + struct intel_digital_port *intel_dig_port = > > > dp_to_dig_port(intel_dp); > > > + struct drm_i915_private *dev_priv = > > > + to_i915(intel_dig_port->base.base.dev); > > > + i915_reg_t ch_ctl; > > > + uint32_t status; > > > + int try; > > > + > > > + ch_ctl = intel_dp->aux_ch_ctl_reg(intel_dp); > > > + > > > + for (try = 0; try < 3; try++) { > > > + status = I915_READ_NOTRACE(ch_ctl); > > > + if ((status & DP_AUX_CH_CTL_SEND_BUSY) == 0) > > > + break; > > Did you mean to return false here? > Oh thanks, I just fixed this in the next patch. > > > > > > > Anyway, this code here looks very similar to > > intel_dp_aux_wait_done(). > > Might be worth checking if it can be reused. > I'm not sure that hardware will send a interruption when it finish a > aux ch transaction that started by it self, that is why I'm going > safe > here and just keep what is working for now. I should have clarified I meant intel_dp_aux_wait_done(intel_dp, false).
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 701963a192ee..a11465c62950 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -1062,6 +1062,38 @@ static uint32_t skl_get_aux_send_ctl(struct intel_dp *intel_dp, DP_AUX_CH_CTL_SYNC_PULSE_SKL(32); } +static bool intel_dp_aux_is_busy(struct intel_dp *intel_dp) +{ + struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); + struct drm_i915_private *dev_priv = + to_i915(intel_dig_port->base.base.dev); + i915_reg_t ch_ctl; + uint32_t status; + int try; + + ch_ctl = intel_dp->aux_ch_ctl_reg(intel_dp); + + for (try = 0; try < 3; try++) { + status = I915_READ_NOTRACE(ch_ctl); + if ((status & DP_AUX_CH_CTL_SEND_BUSY) == 0) + break; + msleep(1); + } + + if (try == 3) { + static u32 last_status = -1; + const u32 status = I915_READ(ch_ctl); + + if (status != last_status) { + WARN(1, "dp_aux_ch not started status 0x%08x\n", + status); + last_status = status; + } + } + + return true; +} + static int intel_dp_aux_xfer(struct intel_dp *intel_dp, const uint8_t *send, int send_bytes, @@ -1074,7 +1106,7 @@ intel_dp_aux_xfer(struct intel_dp *intel_dp, i915_reg_t ch_ctl, ch_data[5]; uint32_t aux_clock_divider; int i, ret, recv_bytes; - uint32_t status; + uint32_t status = 0; int try, clock = 0; bool has_aux_irq = HAS_AUX_IRQ(dev_priv); bool vdd; @@ -1102,23 +1134,7 @@ intel_dp_aux_xfer(struct intel_dp *intel_dp, intel_dp_check_edp(intel_dp); /* Try to wait for any previous AUX channel activity */ - for (try = 0; try < 3; try++) { - status = I915_READ_NOTRACE(ch_ctl); - if ((status & DP_AUX_CH_CTL_SEND_BUSY) == 0) - break; - msleep(1); - } - - if (try == 3) { - static u32 last_status = -1; - const u32 status = I915_READ(ch_ctl); - - if (status != last_status) { - WARN(1, "dp_aux_ch not started status 0x%08x\n", - status); - last_status = status; - } - + if (intel_dp_aux_is_busy(intel_dp)) { ret = -EBUSY; goto out; }
This reduces the spaghetti that intel_dp_aux_xfer(). Moved doing less changes possible here, improvements to the new function in further patch. Signed-off-by: José Roberto de Souza <jose.souza@intel.com> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> --- New patch in this series. drivers/gpu/drm/i915/intel_dp.c | 52 +++++++++++++++++++++------------ 1 file changed, 34 insertions(+), 18 deletions(-)