Re: [PATCH] drm/msm/dp: do not complete dp_aux_cmd_fifo_tx() if irq is not for aux transfer

Thu, 15 Dec 2022 09:12:52 -0800 


On 12/14/2022 6:59 PM, Abhinav Kumar wrote:

Hi Stephen

On 12/14/2022 4:29 PM, Stephen Boyd wrote:

Quoting Doug Anderson (2022-12-14 16:14:42)

Hi,


On Wed, Dec 14, 2022 at 3:46 PM Abhinav Kumar 
<quic_abhin...@quicinc.com> wrote:


Hi Doug

On 12/14/2022 2:29 PM, Doug Anderson wrote:

Hi,


On Wed, Dec 14, 2022 at 1:21 PM Kuogee Hsieh 
<quic_khs...@quicinc.com> wrote:


There are 3 possible interrupt sources are handled by DP controller,
HPDstatus, Controller state changes and Aux read/write transaction.

At every irq, DP controller have to check isr status of every 
interrupt
sources and service the interrupt if its isr status bits shows 
interrupts
are pending. There is potential race condition may happen at 
current aux
isr handler implementation since it is always complete 
dp_aux_cmd_fifo_tx()
even irq is not for aux read or write transaction. This may cause 
aux read
transaction return premature if host aux data read is in the 
middle of
waiting for sink to complete transferring data to host while irq 
happen.
This will cause host's receiving buffer contains unexpected data. 
This
patch fixes this problem by checking aux isr and return 
immediately at

aux isr handler if there are no any isr status bits set.

Follows are the signature at kernel logs when problem happen,
EDID has corrupt header

panel-simple-dp-aux aux-aea0000.edp: Couldn't identify panel via 
EDID
panel-simple-dp-aux aux-aea0000.edp: error -EIO: Couldn't detect 
panel nor find a fallback


Signed-off-by: Kuogee Hsieh <quic_khs...@quicinc.com>
---
   drivers/gpu/drm/msm/dp/dp_aux.c | 7 +++++++
   1 file changed, 7 insertions(+)


diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c 
b/drivers/gpu/drm/msm/dp/dp_aux.c

index d030a93..8f8b12a 100644
--- a/drivers/gpu/drm/msm/dp/dp_aux.c
+++ b/drivers/gpu/drm/msm/dp/dp_aux.c
@@ -423,6 +423,13 @@ void dp_aux_isr(struct drm_dp_aux *dp_aux)

          isr = dp_catalog_aux_get_irq(aux->catalog);

+       /*
+        * if this irq is not for aux transfer,
+        * then return immediately
+        */


Why do you need 4 lines for a comment that fits on one line?

Yes, we can fit this to one line.



+       if (!isr)
+               return;


I can confirm that this works for me. I could reproduce the EDID
problems in the past and I can't after this patch. ...so I could give
a:

Tested-by: Douglas Anderson <diand...@chromium.org>

I'm not an expert on this part of the code, so feel free to ignore my
other comments if everyone else thinks this patch is fine as-is, but
to me something here feels a little fragile. It feels a little weird
that we'll "complete" for _any_ interrupt that comes through now
rather than relying on dp_aux_native_handler() / dp_aux_i2c_handler()
to specifically identify interrupts that caused the end of the
transfer. I guess that idea is that every possible interrupt we get
causes the end of the transfer?

-Doug



So this turned out to be more tricky and was a good finding from 
kuogee.


In the bad EDID case, it was technically not bad EDID.

What was happening was, the VIDEO_READY interrupt was continuously
firing. Ideally, this should fire only once but due to some error
condition it kept firing. We dont exactly know why yet what was the
error condition making it continuously fire.


This is a great detail that is missing from the commit text.


Yup, we should update this.


In the DP ISR, the dp_aux_isr() gets called even if it was not an aux
interrupt which fired (so the call flow in this case was
dp_display_irq_handler (triggered for VIDEO_READY) ---> dp_aux_isr()
So we should certainly have some protection to return early from this
routine if there was no aux interrupt which fired.


I'm not sure that's a race condition though, more like a problem where
the completion is called unconditionally?



hmm ... True.



Which is what this fix is doing.

Its not completing any interrupt, its just returning early if no aux
interrupt fired.


...but the whole problem was that it was doing the complete() at the
end, right? Kuogee even mentioned that in the commit message.
Specifically, I checked dp_aux_native_handler() and
dp_aux_i2c_handler(), both of which are passed the "isr". Unless I
messed up, both functions already were no-ops if the ISR was 0, even
before Kuogee's patch. That means that the only thing Kuogee's patch
does is to prevent the call to "complete(&aux->comp)" at the end of
"dp_aux_isr()".

...and it makes sense not to call the complete() if no "isr" is 0.
...but what I'm saying is that _any_ non-zero value of ISR will still
cause the complete() to be called after Kuogee's patch. That means
that if any of the 32-bits in the "isr" variable are set, that we will
call complete(). I'm asking if you're sure that every single bit of
the "isr" means that we're ready to call complete(). It feels like it
would be less fragile if dp_aux_native_handler() and
dp_aux_i2c_handler() (which both already look at the ISR) returned
some value saying whether the "isr" contained a bit that meant that
complete() should be called.



I'm almost certain I've asked for this before, but I can't find it
anymore. Can we also simplify the aux handlers to be a big pile of
if-else-if conditions that don't overwrite the 'aux_error_num'? That
would simplify the patch below.



Okay, this makes it much clear about what Doug was trying to explain.

This certainly improves the irq return code better (handled Vs none).


In terms of the functionality of it, although it looks too simple, the 
current change was okay.



But, I agree that this would be more robust in terms of the irq return 
codes.


Kuogee, any concerns with this?

no, let me improve the aux is handler.



---8<---

diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c 
b/drivers/gpu/drm/msm/dp/dp_aux.c

index d030a93a08c3..ff79cad90d21 100644
--- a/drivers/gpu/drm/msm/dp/dp_aux.c
+++ b/drivers/gpu/drm/msm/dp/dp_aux.c
@@ -162,45 +162,73 @@ static ssize_t dp_aux_cmd_fifo_rx(struct
dp_aux_private *aux,
      return i;
  }

-static void dp_aux_native_handler(struct dp_aux_private *aux, u32 isr)

+static irqreturn_t dp_aux_native_handler(struct dp_aux_private *aux, 
u32 isr)

  {
-    if (isr & DP_INTR_AUX_I2C_DONE)
+    irqreturn_t ret = IRQ_NONE;
+
+    if (isr & DP_INTR_AUX_I2C_DONE) {
          aux->aux_error_num = DP_AUX_ERR_NONE;
-    else if (isr & DP_INTR_WRONG_ADDR)
+        ret = IRQ_HANDLED;
+    } else if (isr & DP_INTR_WRONG_ADDR) {
          aux->aux_error_num = DP_AUX_ERR_ADDR;
-    else if (isr & DP_INTR_TIMEOUT)
+        ret = IRQ_HANDLED;
+    } else if (isr & DP_INTR_TIMEOUT) {
          aux->aux_error_num = DP_AUX_ERR_TOUT;
-    if (isr & DP_INTR_NACK_DEFER)
+        ret = IRQ_HANDLED;
+    }
+
+    if (isr & DP_INTR_NACK_DEFER) {
          aux->aux_error_num = DP_AUX_ERR_NACK;
+        ret = IRQ_HANDLED;
+    }
      if (isr & DP_INTR_AUX_ERROR) {
          aux->aux_error_num = DP_AUX_ERR_PHY;
          dp_catalog_aux_clear_hw_interrupts(aux->catalog);
+        ret = IRQ_HANDLED;
      }
+
+    return ret;
  }

-static void dp_aux_i2c_handler(struct dp_aux_private *aux, u32 isr)

+static irqreturn_t dp_aux_i2c_handler(struct dp_aux_private *aux, 
u32 isr)

  {
+    irqreturn_t ret = IRQ_NONE;
+
      if (isr & DP_INTR_AUX_I2C_DONE) {
          if (isr & (DP_INTR_I2C_NACK | DP_INTR_I2C_DEFER))
              aux->aux_error_num = DP_AUX_ERR_NACK;
          else
              aux->aux_error_num = DP_AUX_ERR_NONE;
-    } else {
-        if (isr & DP_INTR_WRONG_ADDR)
-            aux->aux_error_num = DP_AUX_ERR_ADDR;
-        else if (isr & DP_INTR_TIMEOUT)
-            aux->aux_error_num = DP_AUX_ERR_TOUT;
-        if (isr & DP_INTR_NACK_DEFER)
-            aux->aux_error_num = DP_AUX_ERR_NACK_DEFER;
-        if (isr & DP_INTR_I2C_NACK)
-            aux->aux_error_num = DP_AUX_ERR_NACK;
-        if (isr & DP_INTR_I2C_DEFER)
-            aux->aux_error_num = DP_AUX_ERR_DEFER;
-        if (isr & DP_INTR_AUX_ERROR) {
-            aux->aux_error_num = DP_AUX_ERR_PHY;
- dp_catalog_aux_clear_hw_interrupts(aux->catalog);
-        }
+        return IRQ_HANDLED;
+    }
+
+    if (isr & DP_INTR_WRONG_ADDR) {
+        aux->aux_error_num = DP_AUX_ERR_ADDR;
+        ret = IRQ_HANDLED;
+    } else if (isr & DP_INTR_TIMEOUT) {
+        aux->aux_error_num = DP_AUX_ERR_TOUT;
+        ret = IRQ_HANDLED;
+    }
+
+    if (isr & DP_INTR_NACK_DEFER) {
+        aux->aux_error_num = DP_AUX_ERR_NACK_DEFER;
+        ret = IRQ_HANDLED;
+    }
+    if (isr & DP_INTR_I2C_NACK) {
+        aux->aux_error_num = DP_AUX_ERR_NACK;
+        ret = IRQ_HANDLED;
+    }
+    if (isr & DP_INTR_I2C_DEFER) {
+        aux->aux_error_num = DP_AUX_ERR_DEFER;
+        ret = IRQ_HANDLED;
+    }
+    if (isr & DP_INTR_AUX_ERROR) {
+        aux->aux_error_num = DP_AUX_ERR_PHY;
+        dp_catalog_aux_clear_hw_interrupts(aux->catalog);
+        ret = IRQ_HANDLED;
      }
+
+    return ret;
  }


  static void dp_aux_update_offset_and_segment(struct dp_aux_private 
*aux,
@@ -409,14 +437,15 @@ static ssize_t dp_aux_transfer(struct 
drm_dp_aux *dp_aux,

      return ret;
  }

-void dp_aux_isr(struct drm_dp_aux *dp_aux)
+irqreturn_t dp_aux_isr(struct drm_dp_aux *dp_aux)
  {
      u32 isr;
      struct dp_aux_private *aux;
+    irqreturn_t ret = IRQ_NONE;

      if (!dp_aux) {
          DRM_ERROR("invalid input\n");
-        return;
+        return ret;
      }

      aux = container_of(dp_aux, struct dp_aux_private, dp_aux);
@@ -424,14 +453,17 @@ void dp_aux_isr(struct drm_dp_aux *dp_aux)
      isr = dp_catalog_aux_get_irq(aux->catalog);

      if (!aux->cmd_busy)
-        return;
+        return ret;

      if (aux->native)
-        dp_aux_native_handler(aux, isr);
+        ret |= dp_aux_native_handler(aux, isr);
      else
-        dp_aux_i2c_handler(aux, isr);
+        ret |= dp_aux_i2c_handler(aux, isr);

-    complete(&aux->comp);
+    if (ret == IRQ_HANDLED)
+        complete(&aux->comp);
+
+    return ret;
  }

  void dp_aux_reconfig(struct drm_dp_aux *dp_aux)

diff --git a/drivers/gpu/drm/msm/dp/dp_aux.h 
b/drivers/gpu/drm/msm/dp/dp_aux.h

index e930974bcb5b..511305da4f66 100644
--- a/drivers/gpu/drm/msm/dp/dp_aux.h
+++ b/drivers/gpu/drm/msm/dp/dp_aux.h
@@ -11,7 +11,7 @@

  int dp_aux_register(struct drm_dp_aux *dp_aux);
  void dp_aux_unregister(struct drm_dp_aux *dp_aux);
-void dp_aux_isr(struct drm_dp_aux *dp_aux);
+irqreturn_t dp_aux_isr(struct drm_dp_aux *dp_aux);
  void dp_aux_init(struct drm_dp_aux *dp_aux);
  void dp_aux_deinit(struct drm_dp_aux *dp_aux);
  void dp_aux_reconfig(struct drm_dp_aux *dp_aux);

diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c 
b/drivers/gpu/drm/msm/dp/dp_ctrl.c

index dd26ca651a05..10c6d6847163 100644
--- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
+++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
@@ -1979,13 +1979,11 @@ int dp_ctrl_off(struct dp_ctrl *dp_ctrl)
      return ret;
  }

-void dp_ctrl_isr(struct dp_ctrl *dp_ctrl)
+irqreturn_t dp_ctrl_isr(struct dp_ctrl *dp_ctrl)
  {
      struct dp_ctrl_private *ctrl;
      u32 isr;
-
-    if (!dp_ctrl)
-        return;
+    irqreturn_t ret = IRQ_NONE;

      ctrl = container_of(dp_ctrl, struct dp_ctrl_private, dp_ctrl);

@@ -1994,12 +1992,16 @@ void dp_ctrl_isr(struct dp_ctrl *dp_ctrl)
      if (isr & DP_CTRL_INTR_READY_FOR_VIDEO) {
          drm_dbg_dp(ctrl->drm_dev, "dp_video_ready\n");
          complete(&ctrl->video_comp);
+        ret = IRQ_HANDLED;
      }

      if (isr & DP_CTRL_INTR_IDLE_PATTERN_SENT) {
          drm_dbg_dp(ctrl->drm_dev, "idle_patterns_sent\n");
          complete(&ctrl->idle_comp);
+        ret = IRQ_HANDLED;
      }
+
+    return ret;
  }

  struct dp_ctrl *dp_ctrl_get(struct device *dev, struct dp_link *link,

diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.h 
b/drivers/gpu/drm/msm/dp/dp_ctrl.h

index 9f29734af81c..c3af06dc87b1 100644
--- a/drivers/gpu/drm/msm/dp/dp_ctrl.h
+++ b/drivers/gpu/drm/msm/dp/dp_ctrl.h
@@ -25,7 +25,7 @@ int dp_ctrl_off_link_stream(struct dp_ctrl *dp_ctrl);
  int dp_ctrl_off_link(struct dp_ctrl *dp_ctrl);
  int dp_ctrl_off(struct dp_ctrl *dp_ctrl);
  void dp_ctrl_push_idle(struct dp_ctrl *dp_ctrl);
-void dp_ctrl_isr(struct dp_ctrl *dp_ctrl);
+irqreturn_t dp_ctrl_isr(struct dp_ctrl *dp_ctrl);
  void dp_ctrl_handle_sink_request(struct dp_ctrl *dp_ctrl);
  struct dp_ctrl *dp_ctrl_get(struct device *dev, struct dp_link *link,
              struct dp_panel *panel,    struct drm_dp_aux *aux,
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
b/drivers/gpu/drm/msm/dp/dp_display.c
index a49f6dbbe888..559d9ab7954d 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -1192,7 +1192,7 @@ static int dp_hpd_event_thread_start(struct
dp_display_private *dp_priv)
  static irqreturn_t dp_display_irq_handler(int irq, void *dev_id)
  {
      struct dp_display_private *dp = dev_id;
-    irqreturn_t ret = IRQ_HANDLED;
+    irqreturn_t ret = IRQ_NONE;
      u32 hpd_isr_status;

      if (!dp) {
@@ -1206,27 +1206,33 @@ static irqreturn_t dp_display_irq_handler(int
irq, void *dev_id)
          drm_dbg_dp(dp->drm_dev, "type=%d isr=0x%x\n",
              dp->dp_display.connector_type, hpd_isr_status);
          /* hpd related interrupts */
-        if (hpd_isr_status & DP_DP_HPD_PLUG_INT_MASK)
+        if (hpd_isr_status & DP_DP_HPD_PLUG_INT_MASK) {
              dp_add_event(dp, EV_HPD_PLUG_INT, 0, 0);
+            ret = IRQ_HANDLED;
+        }

          if (hpd_isr_status & DP_DP_IRQ_HPD_INT_MASK) {
              dp_add_event(dp, EV_IRQ_HPD_INT, 0, 0);
+            ret = IRQ_HANDLED;
          }

          if (hpd_isr_status & DP_DP_HPD_REPLUG_INT_MASK) {
              dp_add_event(dp, EV_HPD_UNPLUG_INT, 0, 0);
              dp_add_event(dp, EV_HPD_PLUG_INT, 0, 3);
+            ret = IRQ_HANDLED;
          }

-        if (hpd_isr_status & DP_DP_HPD_UNPLUG_INT_MASK)
+        if (hpd_isr_status & DP_DP_HPD_UNPLUG_INT_MASK) {
              dp_add_event(dp, EV_HPD_UNPLUG_INT, 0, 0);
+            ret = IRQ_HANDLED;
+        }
      }

      /* DP controller isr */
-    dp_ctrl_isr(dp->ctrl);
+    ret |= dp_ctrl_isr(dp->ctrl);

      /* DP aux isr */
-    dp_aux_isr(dp->aux);
+    ret |= dp_aux_isr(dp->aux);

      return ret;
  }






















					Reply via email to