Re: [PATCH v4 2/2] drm/msm/dp: enhance dp controller isr

From: Dmitry Baryshkov
Date: Thu Dec 15 2022 - 18:11:57 EST

Next message: Yury Norov: "[GIT PULL] bitmap changes for v6.2-rc1"
Previous message: Paul E. McKenney: "Re: [PATCH] tracing: Do not synchronize freeing of trigger filter on boot up"
In reply to: Kuogee Hsieh: "[PATCH v4 2/2] drm/msm/dp: enhance dp controller isr"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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

Next message: Yury Norov: "[GIT PULL] bitmap changes for v6.2-rc1"
Previous message: Paul E. McKenney: "Re: [PATCH] tracing: Do not synchronize freeing of trigger filter on boot up"
In reply to: Kuogee Hsieh: "[PATCH v4 2/2] drm/msm/dp: enhance dp controller isr"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]