Re: [Intel-gfx] [PATCH v2 7/9] drm/i915/dp: Move code to check if aux ch is busy to a function

[Pandiyan, Dhinakaran]

On Mon, 2018-04-30 at 23:39 +, 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 
> > > Cc: Dhinakaran Pandiyan 
> > > ---
> > > 
> > > 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).
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v2 7/9] drm/i915/dp: Move code to check if aux ch is busy to a function

[Souza, Jose]

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 
> > Cc: Dhinakaran Pandiyan 
> > ---
> > 
> > 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;
> > }
> 
> 
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v2 7/9] drm/i915/dp: Move code to check if aux ch is busy to a function

[Dhinakaran Pandiyan]

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 
> Cc: Dhinakaran Pandiyan 
> ---
> 
> 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;
>   }

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH v2 7/9] drm/i915/dp: Move code to check if aux ch is busy to a function

[José Roberto de Souza]

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 
Cc: Dhinakaran Pandiyan 
---

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;
+   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;
}
-- 
2.17.0

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx