On Fri, 16 Dec 2022 at 00:53, Kuogee Hsieh <quic_khsieh@xxxxxxxxxxx> 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. > > Signed-off-by: Kuogee Hsieh <quic_khsieh@xxxxxxxxxxx> > Suggested-by: Stephen Boyd <swboyd@xxxxxxxxxxxx> > --- > drivers/gpu/drm/msm/dp/dp_aux.c | 96 ++++++++++++++++++++++++------------- > drivers/gpu/drm/msm/dp/dp_aux.h | 2 +- > drivers/gpu/drm/msm/dp/dp_ctrl.c | 13 +++-- > drivers/gpu/drm/msm/dp/dp_ctrl.h | 2 +- > drivers/gpu/drm/msm/dp/dp_display.c | 16 +++++-- > 5 files changed, 86 insertions(+), 43 deletions(-) > > diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c b/drivers/gpu/drm/msm/dp/dp_aux.c > index cc3efed..2b85b61 100644 > --- a/drivers/gpu/drm/msm/dp/dp_aux.c > +++ b/drivers/gpu/drm/msm/dp/dp_aux.c > @@ -162,45 +162,78 @@ 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,15 +442,11 @@ 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; > - } Any reason to remove this? It doesn't belong to the IRQ patch. > + irqreturn_t ret = IRQ_NONE; Just irqreturn_t ret; > > aux = container_of(dp_aux, struct dp_aux_private, dp_aux); > > @@ -425,17 +454,20 @@ void dp_aux_isr(struct drm_dp_aux *dp_aux) > > /* no interrupts pending, return immediately */ > if (!isr) > - return; > + return ret; return IRQ_NONE; > > if (!aux->cmd_busy) > - return; > + return ret; return IRQ_NONE; > > 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); I still think that it would be better to move complete() to the i2c and native handlers. Then you can write something as simple as: if (aux->native) return dp_aux_native_handler(...); return dp_aux_i2c_handler(....); > + > + 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 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..5968ab1 100644 > --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c > +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c > @@ -1982,27 +1982,32 @@ 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; Again, why? > + irqreturn_t ret = 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 ret; Please apply the same pattern here. If you know that you are returning IRQ_NONE, return it directly. > > 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; > } > -- > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, > a Linux Foundation Collaborative Project > -- With best wishes Dmitry