| Message ID | 1672193785-11003-3-git-send-email-quic_khsieh@quicinc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | do not complete dp_aux_cmd_fifo_tx() if irq is not for aux transfer | expand |
On 28/12/2022 04:16, Kuogee Hsieh wrote: > dp_display_irq_handler() is the main isr handler with the helps > of two sub isr, dp_aux_isr and dp_ctrl_isr, to service all DP > interrupts on every irq triggered. Current all three isr does > not return IRQ_HANDLED if there are any interrupts it had > serviced. This patch fix this ambiguity by having all isr > return IRQ_HANDLED if there are interrupts had been serviced > or IRQ_NONE otherwise. > > Changes in v5: > -- move complete into dp_aux_native_handler() > -- move complete into dp_aux_i2c_handler() > -- restore null ctrl check at isr > -- return IRQ_NODE directly > > Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com> > Suggested-by: Stephen Boyd <swboyd@chromium.org> > --- > drivers/gpu/drm/msm/dp/dp_aux.c | 95 ++++++++++++++++++++++++++----------- > drivers/gpu/drm/msm/dp/dp_aux.h | 2 +- > drivers/gpu/drm/msm/dp/dp_ctrl.c | 12 ++++- > drivers/gpu/drm/msm/dp/dp_ctrl.h | 2 +- > drivers/gpu/drm/msm/dp/dp_display.c | 16 +++++-- > 5 files changed, 89 insertions(+), 38 deletions(-) > Stephen, Dough, do we still want this patch in?
Hi, On Tue, Dec 27, 2022 at 6:16 PM Kuogee Hsieh <quic_khsieh@quicinc.com> wrote: > > dp_display_irq_handler() is the main isr handler with the helps > of two sub isr, dp_aux_isr and dp_ctrl_isr, to service all DP > interrupts on every irq triggered. Current all three isr does > not return IRQ_HANDLED if there are any interrupts it had > serviced. This patch fix this ambiguity by having all isr > return IRQ_HANDLED if there are interrupts had been serviced > or IRQ_NONE otherwise. > > Changes in v5: > -- move complete into dp_aux_native_handler() > -- move complete into dp_aux_i2c_handler() > -- restore null ctrl check at isr > -- return IRQ_NODE directly > > Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com> > Suggested-by: Stephen Boyd <swboyd@chromium.org> > --- > drivers/gpu/drm/msm/dp/dp_aux.c | 95 ++++++++++++++++++++++++++----------- > drivers/gpu/drm/msm/dp/dp_aux.h | 2 +- > drivers/gpu/drm/msm/dp/dp_ctrl.c | 12 ++++- > drivers/gpu/drm/msm/dp/dp_ctrl.h | 2 +- > drivers/gpu/drm/msm/dp/dp_display.c | 16 +++++-- > 5 files changed, 89 insertions(+), 38 deletions(-) > > diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c b/drivers/gpu/drm/msm/dp/dp_aux.c > index cc3efed..d01ff45 100644 > --- a/drivers/gpu/drm/msm/dp/dp_aux.c > +++ b/drivers/gpu/drm/msm/dp/dp_aux.c > @@ -162,45 +162,84 @@ 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; > } The end result of the above is a weird mix of "if" and "else if" for no apparent reason. All except one of them just updates the exact same variable so doing more than one is mostly useless. If you made it consistently with "else" then the whole thing could be much easier, like this (untested): static irqreturn_t dp_aux_native_handler(struct dp_aux_private *aux, u32 isr) { if (isr & DP_INTR_AUX_ERROR) { aux->aux_error_num = DP_AUX_ERR_PHY; dp_catalog_aux_clear_hw_interrupts(aux->catalog); } else if (isr & DP_INTR_NACK_DEFER) { aux->aux_error_num = DP_AUX_ERR_NACK; } else if (isr & DP_INTR_AUX_I2C_DONE) { 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; } else { return IRQ_NONE; } complete(&aux->comp); return IRQ_HANDLED; } Note that I changed the order to make sure that the behavior was exactly the same (previously later tests without the "if" would override "aux_error_num" so I moved them first. Also previously dp_catalog_aux_clear_hw_interrupts() would always be called for the PHY error even if other errors were present so my new proposal preserves this behavior. > + > + if (ret == IRQ_HANDLED) > + complete(&aux->comp); > + > + 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; It's hard to see in "diff" form, but if you apply your patch and check I think there's a bug. Specifically if DP_INTR_AUX_I2C_DONE is found then we'll return IRQ_HANDLED without completing. Also: same comment as with the above function, this is all cleaner if you just consistently use "else if". Untested: static irqreturn_t dp_aux_i2c_handler(struct dp_aux_private *aux, u32 isr) { 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_AUX_ERROR) { aux->aux_error_num = DP_AUX_ERR_PHY; dp_catalog_aux_clear_hw_interrupts(aux->catalog); } else if (isr & DP_INTR_I2C_DEFER) { aux->aux_error_num = DP_AUX_ERR_DEFER; } else if (isr & DP_INTR_I2C_NACK) { aux->aux_error_num = DP_AUX_ERR_NACK; } else if (isr & DP_INTR_NACK_DEFER) { aux->aux_error_num = DP_AUX_ERR_NACK_DEFER; } 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; } else { return IRQ_NONE; } complete(&aux->comp); 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; > + } > + > + if (ret == IRQ_HANDLED) > + complete(&aux->comp); > + > + return ret; > } > > static void dp_aux_update_offset_and_segment(struct dp_aux_private *aux, > @@ -409,14 +448,14 @@ 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; > > if (!dp_aux) { > DRM_ERROR("invalid input\n"); > - return; > + return IRQ_NONE; > } > > aux = container_of(dp_aux, struct dp_aux_private, dp_aux); > @@ -425,17 +464,15 @@ void dp_aux_isr(struct drm_dp_aux *dp_aux) > > /* no interrupts pending, return immediately */ > if (!isr) > - return; > + return IRQ_NONE; > > if (!aux->cmd_busy) > - return; > + return IRQ_NONE; > > if (aux->native) > - dp_aux_native_handler(aux, isr); > + return dp_aux_native_handler(aux, isr); > else > - dp_aux_i2c_handler(aux, isr); > - > - complete(&aux->comp); > + return dp_aux_i2c_handler(aux, isr); Personally, I wouldn't have done it this way. I guess that means I disagree with Stephen. I'm not dead-set against this way and it's fine if you want to continue with it. If I were doing it, though, then I would always return IRQ_HANDLED IF dp_catalog_aux_get_irq() returned anything non-zero. Why? Officially if dp_catalog_aux_get_irq() returns something non-zero then you know for sure that there was an interrupt for this device and officially you have "handled" it by acking it, since dp_catalog_aux_get_irq() acks all the bits that it returns. That means that even if dp_aux_native_handler() or dp_aux_i2c_handler() didn't do anything with the interrupt you at least know that it was for us (so if the IRQ is shared we properly report back to the IRQ subsystem) and that it won't keep firing over and over (because we acked it). NOTE: I still like having the complete() call in dp_aux_native_handler() and dp_aux_i2c_handler() and, to me, that part of this patch is worthwhile. That makes it more obvious that the code is truly expecting that complete to be called for all error cases as well as transfer finished. -Doug
Quoting Doug Anderson (2023-01-18 10:29:59) > Hi, > > On Tue, Dec 27, 2022 at 6:16 PM Kuogee Hsieh <quic_khsieh@quicinc.com> wrote: > > + > > 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; > > } > > The end result of the above is a weird mix of "if" and "else if" for > no apparent reason. All except one of them just updates the exact same > variable so doing more than one is mostly useless. If you made it > consistently with "else" then the whole thing could be much easier, > like this (untested): Totally agreed. I even asked that when I posted the RFC[1]! "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." > > @@ -425,17 +464,15 @@ void dp_aux_isr(struct drm_dp_aux *dp_aux) > > > > /* no interrupts pending, return immediately */ > > if (!isr) > > - return; > > + return IRQ_NONE; > > > > if (!aux->cmd_busy) > > - return; > > + return IRQ_NONE; > > > > if (aux->native) > > - dp_aux_native_handler(aux, isr); > > + return dp_aux_native_handler(aux, isr); > > else > > - dp_aux_i2c_handler(aux, isr); > > - > > - complete(&aux->comp); > > + return dp_aux_i2c_handler(aux, isr); > > Personally, I wouldn't have done it this way. I guess that means I > disagree with Stephen. I'm not dead-set against this way and it's fine > if you want to continue with it. If I were doing it, though, then I > would always return IRQ_HANDLED IF dp_catalog_aux_get_irq() returned > anything non-zero. Why? Officially if dp_catalog_aux_get_irq() returns > something non-zero then you know for sure that there was an interrupt > for this device and officially you have "handled" it by acking it, > since dp_catalog_aux_get_irq() acks all the bits that it returns. That > means that even if dp_aux_native_handler() or dp_aux_i2c_handler() > didn't do anything with the interrupt you at least know that it was > for us (so if the IRQ is shared we properly report back to the IRQ > subsystem) and that it won't keep firing over and over (because we > acked it). I'm primarily concerned with irq storms taking down the system. Can that happen here? If not, then returning IRQ_NONE is not really useful. The overall IRQ for DP looks to be level, because the driver requests the IRQ that way. The aux interrupt status bits look to be edge style interrupts though, because the driver acks them in the handler. I guess that means the edges come in and latch into the interrupt status register so the driver has to ack all of them to drop the IRQ level for the overall DP interrupt? If the driver only acked the bits it looked at instead of all interrupt bits in the register, then the level would never go down for the IRQ if an unhandled interrupt bit was present like 'DP_INTR_PLL_UNLOCKED'. That would mean we would hit spurious IRQ handling very quickly if that interrupt bit was ever seen. But the driver is acking all interrupts, so probably trying to work IRQ_NONE into this code is not very useful? The only thing it would catch is DP_INTR_PLL_UNLOCKED being set over and over again, which seems unlikely. Of course, why is this driver unmasking interrupt bits it doesn't care about? That may be leading to useless interrupt handling in this driver if some interrupt bit is unmasked but never looked at. Can that be fixed in another patch? > > NOTE: I still like having the complete() call in > dp_aux_native_handler() and dp_aux_i2c_handler() and, to me, that part > of this patch is worthwhile. That makes it more obvious that the code > is truly expecting that complete to be called for all error cases as > well as transfer finished. > I think it may be required. We don't want to allow DP_INTR_PLL_UNLOCKED to complete() the transfer. [1] https://lore.kernel.org/all/CAE-0n5100eGC0c09oq4B3M=aHtKW5+wGLGsS1jM91SCyZ5wffQ@mail.gmail.com/
Hi, On Wed, Jan 18, 2023 at 2:34 PM Stephen Boyd <swboyd@chromium.org> wrote: > > Quoting Doug Anderson (2023-01-18 10:29:59) > > Hi, > > > > On Tue, Dec 27, 2022 at 6:16 PM Kuogee Hsieh <quic_khsieh@quicinc.com> wrote: > > > + > > > 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; > > > } > > > > The end result of the above is a weird mix of "if" and "else if" for > > no apparent reason. All except one of them just updates the exact same > > variable so doing more than one is mostly useless. If you made it > > consistently with "else" then the whole thing could be much easier, > > like this (untested): > > Totally agreed. I even asked that when I posted the RFC[1]! > > "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." > > > > @@ -425,17 +464,15 @@ void dp_aux_isr(struct drm_dp_aux *dp_aux) > > > > > > /* no interrupts pending, return immediately */ > > > if (!isr) > > > - return; > > > + return IRQ_NONE; > > > > > > if (!aux->cmd_busy) > > > - return; > > > + return IRQ_NONE; > > > > > > if (aux->native) > > > - dp_aux_native_handler(aux, isr); > > > + return dp_aux_native_handler(aux, isr); > > > else > > > - dp_aux_i2c_handler(aux, isr); > > > - > > > - complete(&aux->comp); > > > + return dp_aux_i2c_handler(aux, isr); > > > > Personally, I wouldn't have done it this way. I guess that means I > > disagree with Stephen. I'm not dead-set against this way and it's fine > > if you want to continue with it. If I were doing it, though, then I > > would always return IRQ_HANDLED IF dp_catalog_aux_get_irq() returned > > anything non-zero. Why? Officially if dp_catalog_aux_get_irq() returns > > something non-zero then you know for sure that there was an interrupt > > for this device and officially you have "handled" it by acking it, > > since dp_catalog_aux_get_irq() acks all the bits that it returns. That > > means that even if dp_aux_native_handler() or dp_aux_i2c_handler() > > didn't do anything with the interrupt you at least know that it was > > for us (so if the IRQ is shared we properly report back to the IRQ > > subsystem) and that it won't keep firing over and over (because we > > acked it). > > I'm primarily concerned with irq storms taking down the system. Can that > happen here? If not, then returning IRQ_NONE is not really useful. The > overall IRQ for DP looks to be level, because the driver requests the > IRQ that way. The aux interrupt status bits look to be edge style > interrupts though, because the driver acks them in the handler. I guess > that means the edges come in and latch into the interrupt status > register so the driver has to ack all of them to drop the IRQ level for > the overall DP interrupt? If the driver only acked the bits it looked at > instead of all interrupt bits in the register, then the level would > never go down for the IRQ if an unhandled interrupt bit was present like > 'DP_INTR_PLL_UNLOCKED'. That would mean we would hit spurious IRQ > handling very quickly if that interrupt bit was ever seen. > > But the driver is acking all interrupts, so probably trying to work > IRQ_NONE into this code is not very useful? The only thing it would > catch is DP_INTR_PLL_UNLOCKED being set over and over again, which seems > unlikely. Of course, why is this driver unmasking interrupt bits it > doesn't care about? That may be leading to useless interrupt handling in > this driver if some interrupt bit is unmasked but never looked at. Can > that be fixed in another patch? > > > > > NOTE: I still like having the complete() call in > > dp_aux_native_handler() and dp_aux_i2c_handler() and, to me, that part > > of this patch is worthwhile. That makes it more obvious that the code > > is truly expecting that complete to be called for all error cases as > > well as transfer finished. > > > > I think it may be required. We don't want to allow DP_INTR_PLL_UNLOCKED > to complete() the transfer. OK, I've tried to code up what I think is the right solution. I'd appreciate review and testing. https://lore.kernel.org/r/20230119145248.1.I90ffed3ddd21e818ae534f820cb4d6d8638859ab@changeid -Doug
diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c b/drivers/gpu/drm/msm/dp/dp_aux.c index cc3efed..d01ff45 100644 --- a/drivers/gpu/drm/msm/dp/dp_aux.c +++ b/drivers/gpu/drm/msm/dp/dp_aux.c @@ -162,45 +162,84 @@ 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; } + + if (ret == IRQ_HANDLED) + complete(&aux->comp); + + 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; + } + + if (ret == IRQ_HANDLED) + complete(&aux->comp); + + return ret; } static void dp_aux_update_offset_and_segment(struct dp_aux_private *aux, @@ -409,14 +448,14 @@ 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; if (!dp_aux) { DRM_ERROR("invalid input\n"); - return; + return IRQ_NONE; } aux = container_of(dp_aux, struct dp_aux_private, dp_aux); @@ -425,17 +464,15 @@ void dp_aux_isr(struct drm_dp_aux *dp_aux) /* no interrupts pending, return immediately */ if (!isr) - return; + return IRQ_NONE; if (!aux->cmd_busy) - return; + return IRQ_NONE; if (aux->native) - dp_aux_native_handler(aux, isr); + return dp_aux_native_handler(aux, isr); else - dp_aux_i2c_handler(aux, isr); - - complete(&aux->comp); + return dp_aux_i2c_handler(aux, isr); } 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 e930974..511305d 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 3854c9f..cb0acb1 100644 --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c @@ -1982,27 +1982,35 @@ 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; + irqreturn_t ret = IRQ_NONE; if (!dp_ctrl) - return; + return IRQ_NONE; ctrl = container_of(dp_ctrl, struct dp_ctrl_private, dp_ctrl); isr = dp_catalog_ctrl_get_interrupt(ctrl->catalog); + /* no interrupts pending, return immediately */ + if (!isr) + return IRQ_NONE; 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 9f29734..c3af06d 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 bfd0aef..d40bfbd 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; }
dp_display_irq_handler() is the main isr handler with the helps of two sub isr, dp_aux_isr and dp_ctrl_isr, to service all DP interrupts on every irq triggered. Current all three isr does not return IRQ_HANDLED if there are any interrupts it had serviced. This patch fix this ambiguity by having all isr return IRQ_HANDLED if there are interrupts had been serviced or IRQ_NONE otherwise. Changes in v5: -- move complete into dp_aux_native_handler() -- move complete into dp_aux_i2c_handler() -- restore null ctrl check at isr -- return IRQ_NODE directly Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com> Suggested-by: Stephen Boyd <swboyd@chromium.org> --- drivers/gpu/drm/msm/dp/dp_aux.c | 95 ++++++++++++++++++++++++++----------- drivers/gpu/drm/msm/dp/dp_aux.h | 2 +- drivers/gpu/drm/msm/dp/dp_ctrl.c | 12 ++++- drivers/gpu/drm/msm/dp/dp_ctrl.h | 2 +- drivers/gpu/drm/msm/dp/dp_display.c | 16 +++++-- 5 files changed, 89 insertions(+), 38 deletions(-)