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

From: Doug Anderson
Date: Wed Jan 18 2023 - 13:30:21 EST

Next message: Yazen Ghannam: "Re: [PATCH v1 3/3] x86/MCE/AMD: Handle reassigned bit definitions for CS SMCA"
Previous message: Bjorn Andersson: "Re: [RFC v4 5/5] arm: dts: msm: Add multiport controller node for usb"
In reply to: Dmitry Baryshkov: "Re: [PATCH v6 2/2] drm/msm/dp: enhance dp controller isr"
Next in thread: Stephen Boyd: "Re: [PATCH v6 2/2] drm/msm/dp: enhance dp controller isr"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi,

On Tue, Dec 27, 2022 at 6:16 PM 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.
>
> 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@xxxxxxxxxxx>
> Suggested-by: Stephen Boyd <swboyd@xxxxxxxxxxxx>
> ---
> 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

Next message: Yazen Ghannam: "Re: [PATCH v1 3/3] x86/MCE/AMD: Handle reassigned bit definitions for CS SMCA"
Previous message: Bjorn Andersson: "Re: [RFC v4 5/5] arm: dts: msm: Add multiport controller node for usb"
In reply to: Dmitry Baryshkov: "Re: [PATCH v6 2/2] drm/msm/dp: enhance dp controller isr"
Next in thread: Stephen Boyd: "Re: [PATCH v6 2/2] drm/msm/dp: enhance dp controller isr"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]