Re: [Freedreno] [PATCH v12 2/5] dt-bindings: msm/dp: add data-lanes and link-frequencies property
Quoting Dmitry Baryshkov (2022-12-15 13:12:49) > On 15/12/2022 02:38, Stephen Boyd wrote: > > Quoting Kuogee Hsieh (2022-12-14 14:56:23) > >> > >> Once link training start, then there are no any interactions between > >> controller and phy during link training session. > > > > What do you mean? The DP controller calls phy_configure() and changes > > the link rate. The return value from phy_configure() should be checked > > and link training should skip link rates that aren't supported and/or > > number of lanes that aren't supported. > > I'd toss another coin into the argument. We have previously discussed > using the link-frequencies property in the context of limiting link > speeds for the DSI. There we have both hardware (SoC) limitations and > the board limitations as in some cases the DSI lanes can not sustain > some high rate. I still hope for these patches to materialize at some point. > > For the DP this is more or less the same story. We have the hardware > (SoC, PHY, etc) limitations, but also we have the board/device > limitations. For example some of the board might not be able to support > HBR3 e.g. because of the PCB design. And while it might be logical to > also add the 'max bit rate' support to the eDP & combo PHYs, it > definitely makes sense to be able to limit the rate on the DP <-> > `something' link. Honestly I don't think the PHY even makes sense to put the link rate property. In the case of Trogdor, the DP controller and DP PHY both support all DP link frequencies. The limiting factor is the TCPC redriver that is only rated to support HBR2. We don't describe the TCPC in DT because the EC controls it. This means we have to put the limit *somewhere*, and putting it in the DP output node is the only place we have right now. I would really prefer we put it wherever the limit is, in this case either in the EC node or on the type-c ports. Another nice to have feature would be to support different TCPCs connected to the same DP port. We were considering doing this on Trogdor, where we would have a TCPC rated for HBR2 and another TCPC rated for HBR3 and then detect which TCPC was in use to adjust the supported link rates. We didn't do this though, so the idea got back-burnered. When the SoC is directly wired to a DP connector, I'd expect the connector to have the link rate property. That's because the connector or the traces outside of the SoC will be the part that's limiting the supported frequencies, not the SoC. The graph would need to be walked to find the link rate of course. The PHY could do this just as much as the DP controller could. > > Now, for all the practical purposes this `something' for the DP is the > DP connector, the eDP panel or the USB-C mux (with the possible > redrivers in the middle). > > Thus I'd support Kuogee's proposal to have link-frequencies in the DP's > outbound endpoint. This is the link which will be driven by the data > stream from the Linux point of view. The PHY is linked through the > 'phys' property, but it doesn't participate in the USB-C (or in the > connector/panel) graph. Why doesn't the PHY participate in the graph? The eDP panel could just as easily be connected to the eDP PHY if the PHY participated in the graph. > > Now let's discuss the data lanes. Currently we have them in the DP > property itself. Please correct me if I'm wrong, but I think that we can > drop it for all the practical purposes. I vaguely recall that the driver was checking data-lanes to figure out how many lanes are usable. This is another shortcut taken on Trogdor to work around a lack of complete DP bindings. We only support two lanes of DP on Trogdor. > Judging by the DP compat string > the driver can determine if it uses 2 lanes (eDP) or 4 lanes > (full-featured DP). In case of USB-C when the altmode dictates whether > to use 2 or 4 lanes, the TCPM (Type-C Port Manager) will negotiate the > mode and pin configuration, then inform the DP controller about the > selected amount of lanes. Then DP informs the PHY about the selection > (note, PHY doesn't have control at all in this scenario). > > The only problematic case is the mixed mode ports, which if I understand > correctly, can be configured either to eDP or DP modes. I'm not sure who > specifies and limits the amount of lanes available to the DP controller. > This would depend on where we send the type-c message in the kernel. It really gets to the heart of the question too. Should the PHY be "dumb" and do whatever the controller tells it to do? Or should the PHY be aware of what's going on and take action itself? Note that the data-lanes property is also used to remap lanes. On sc7180 the lane remapping happens in the DP PHY, and then the type-c PHY can flip that too, so if we don't involve the PHY(s) in the graph we'll have to express this information in the DP controller graph and then pass it to the PHY from the controller. Similarly, when we have more dynamic configuration of the type-c PHY, where USB may
[Freedreno] [PATCH v5 2/2] drm/msm/dp: enhance dp controller isr
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
Suggested-by: Stephen Boyd
---
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;
}
+
+ 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;
str
[Freedreno] [PATCH v5 1/2] drm/msm/dp: do not complete dp_aux_cmd_fifo_tx() if irq is not for aux transfer
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.
Current there is a bug report regrading eDP edid corruption happen during
system booting up. After lengthy debugging to found that that VIDEO_READY
interrupt was continuously firing during system booting up which cause
dp_aux_isr() to complete dp_aux_cmd_fifo_tx() prematurely to retrieve data
from aux hardware buffer which is not yet contains complete data transfer
from sink. This cause edid corruption.
Follows are the signature at kernel logs when problem happen,
EDID has corrupt header
panel-simple-dp-aux aux-aea.edp: Couldn't identify panel via EDID
panel-simple-dp-aux aux-aea.edp: error -EIO: Couldn't detect panel nor find
a fallback
Changes in v2:
-- do complete if (ret == IRQ_HANDLED) ay dp-aux_isr()
-- add more commit text
Changes in v3:
-- add Stephen suggested
-- dp_aux_isr() return IRQ_XXX back to caller
-- dp_ctrl_isr() return IRQ_XXX back to caller
Changes in v4:
-- split into two patches
Changes in v5:
-- delete empty line between tags
Fixes: c943b4948b58 ("drm/msm/dp: add displayPort driver support")
Signed-off-by: Kuogee Hsieh
Tested-by: Douglas Anderson
Reviewed-by: Abhinav Kumar
Reviewed-by: Dmitry Baryshkov
---
drivers/gpu/drm/msm/dp/dp_aux.c | 4
1 file changed, 4 insertions(+)
diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c b/drivers/gpu/drm/msm/dp/dp_aux.c
index d030a93..cc3efed 100644
--- a/drivers/gpu/drm/msm/dp/dp_aux.c
+++ b/drivers/gpu/drm/msm/dp/dp_aux.c
@@ -423,6 +423,10 @@ void dp_aux_isr(struct drm_dp_aux *dp_aux)
isr = dp_catalog_aux_get_irq(aux->catalog);
+ /* no interrupts pending, return immediately */
+ if (!isr)
+ return;
+
if (!aux->cmd_busy)
return;
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
[Freedreno] [PATCH v5 0/2] do not complete dp_aux_cmd_fifo_tx() if irq is not for aux transfer
ignore spuriors isr at dp_aux_isr() to fixed eDP edid read failed Kuogee Hsieh (2): drm/msm/dp: do not complete dp_aux_cmd_fifo_tx() if irq is not for aux transfer drm/msm/dp: enhance dp controller isr drivers/gpu/drm/msm/dp/dp_aux.c | 97 ++--- 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, 92 insertions(+), 37 deletions(-) -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
Re: [Freedreno] [PATCH v4 2/2] drm/msm/dp: enhance dp controller isr
On Fri, 16 Dec 2022 at 00:53, 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.
>
> Signed-off-by: Kuogee Hsieh
> Suggested-by: Stephen Boyd
> ---
> 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;
> s
Re: [Freedreno] [PATCH v4 1/2] drm/msm/dp: do not complete dp_aux_cmd_fifo_tx() if irq is not for aux transfer
On Fri, 16 Dec 2022 at 00:53, Kuogee Hsieh 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.
>
> Current there is a bug report regrading eDP edid corruption happen during
> system booting up. After lengthy debugging to found that that VIDEO_READY
> interrupt was continuously firing during system booting up which cause
> dp_aux_isr() to complete dp_aux_cmd_fifo_tx() prematurely to retrieve data
> from aux hardware buffer which is not yet contains complete data transfer
> from sink. This cause edid corruption.
>
> Follows are the signature at kernel logs when problem happen,
> EDID has corrupt header
> panel-simple-dp-aux aux-aea.edp: Couldn't identify panel via EDID
> panel-simple-dp-aux aux-aea.edp: error -EIO: Couldn't detect panel nor
> find a fallback
>
> Changes in v2:
> -- do complete if (ret == IRQ_HANDLED) ay dp-aux_isr()
> -- add more commit text
>
> Changes in v3:
> -- add Stephen suggested
> -- dp_aux_isr() return IRQ_XXX back to caller
> -- dp_ctrl_isr() return IRQ_XXX back to caller
>
> Changes in v4:
> -- split into two patches
>
> Fixes: c943b4948b58 ("drm/msm/dp: add displayPort driver support")
>
The same feedback as the one I gave for v2 or v3:
There should be no empty lines between tags. And you still have one
empty line here.
With that fixed:
Reviewed-by: Dmitry Baryshkov
> Signed-off-by: Kuogee Hsieh
> Tested-by: Douglas Anderson
> Reviewed-by: Abhinav Kumar
> ---
> drivers/gpu/drm/msm/dp/dp_aux.c | 4
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c b/drivers/gpu/drm/msm/dp/dp_aux.c
> index d030a93..cc3efed 100644
> --- a/drivers/gpu/drm/msm/dp/dp_aux.c
> +++ b/drivers/gpu/drm/msm/dp/dp_aux.c
> @@ -423,6 +423,10 @@ void dp_aux_isr(struct drm_dp_aux *dp_aux)
>
> isr = dp_catalog_aux_get_irq(aux->catalog);
>
> + /* no interrupts pending, return immediately */
> + if (!isr)
> + return;
> +
> if (!aux->cmd_busy)
> return;
>
> --
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
>
--
With best wishes
Dmitry
Re: [Freedreno] [PATCH v12 2/5] dt-bindings: msm/dp: add data-lanes and link-frequencies property
Hi,
On Thu, Dec 15, 2022 at 1:12 PM Dmitry Baryshkov
wrote:
>
> On 15/12/2022 02:38, Stephen Boyd wrote:
> > Quoting Kuogee Hsieh (2022-12-14 14:56:23)
> >>
> >> On 12/13/2022 3:06 PM, Stephen Boyd wrote:
> >>> Quoting Kuogee Hsieh (2022-12-13 13:44:05)
> Add both data-lanes and link-frequencies property into endpoint
> >>> Why do we care? Please tell us why it's important.
> >
> > Any response?
> >
> @@ -193,6 +217,8 @@ examples:
> reg = <1>;
> endpoint {
> remote-endpoint = <&typec>;
> +data-lanes = <0 1>;
> +link-frequencies = /bits/ 64 <162000 27
> 54 81>;
> };
> >>> So far we haven't used the output port on the DP controller in DT.
> >>>
> >>> I'm still not clear on what we should do in general for DP because
> >>> there's a PHY that actually controls a lane count and lane mapping. In
> >>> my mental model of the SoC, this DP controller's output port is
> >>> connected to the DP PHY, which then sends the DP lanes out of the SoC to
> >>> the next downstream device (i.e. a DP connector or type-c muxer). Having
> >>> a remote-endpoint property with a phandle to typec doesn't fit my mental
> >>> model. I'd expect it to be the typec PHY.
> >> ack
> >>>
> >>> That brings up the question: when we have 2 lanes vs. 4 lanes will we
> >>> duplicate the data-lanes property in the PHY binding? I suspect we'll
> >>> have to. Hopefully that sort of duplication is OK?
> >> Current we have limitation by reserve 2 data lanes for usb2, i am not
> >> sure duplication to 4 lanes will work automatically.
> >>>
> >>> Similarly, we may have a redriver that limits the link-frequencies
> >>> property further (e.g. only support <= 2.7GHz). Having multiple
> >>> link-frequencies along the graph is OK, right? And isn't the
> >>> link-frequencies property known here by fact that the DP controller
> >>> tells us which SoC this controller is for, and thus we already know the
> >>> supported link frequencies?
> >>>
> >>> Finally, I wonder if we should put any of this in the DP controller's
> >>> output endpoint, or if we can put these sorts of properties in the DP
> >>> PHY binding directly? Can't we do that and then when the DP controller
> >>> tries to set 4 lanes, the PHY immediately fails the call and the link
> >>> training algorithm does its thing and tries fewer lanes? And similarly,
> >>> if link-frequencies were in the PHY's binding, the PHY could fail to set
> >>> those frequencies during link training, returning an error to the DP
> >>> controller, letting the training move on to a lower frequency. If we did
> >>> that this patch series would largely be about modifying the PHY binding,
> >>> updating the PHY driver to enforce constraints, and handling errors
> >>> during link training in the DP controller (which may already be done? I
> >>> didn't check).
> >>
> >>
> >> phy/pll have different configuration base on link lanes and rate.
> >>
> >> it has to be set up before link training can start.
> >>
> >> Once link training start, then there are no any interactions between
> >> controller and phy during link training session.
> >
> > What do you mean? The DP controller calls phy_configure() and changes
> > the link rate. The return value from phy_configure() should be checked
> > and link training should skip link rates that aren't supported and/or
> > number of lanes that aren't supported.
>
> I'd toss another coin into the argument. We have previously discussed
> using the link-frequencies property in the context of limiting link
> speeds for the DSI. There we have both hardware (SoC) limitations and
> the board limitations as in some cases the DSI lanes can not sustain
> some high rate. I still hope for these patches to materialize at some point.
>
> For the DP this is more or less the same story. We have the hardware
> (SoC, PHY, etc) limitations, but also we have the board/device
> limitations. For example some of the board might not be able to support
> HBR3 e.g. because of the PCB design. And while it might be logical to
> also add the 'max bit rate' support to the eDP & combo PHYs, it
> definitely makes sense to be able to limit the rate on the DP <->
> `something' link.
>
> Now, for all the practical purposes this `something' for the DP is the
> DP connector, the eDP panel or the USB-C mux (with the possible
> redrivers in the middle).
>
> Thus I'd support Kuogee's proposal to have link-frequencies in the DP's
> outbound endpoint. This is the link which will be driven by the data
> stream from the Linux point of view. The PHY is linked through the
> 'phys' property, but it doesn't participate in the USB-C (or in the
> connector/panel) graph.
>
> Now let's discuss the data lanes. Currently we have them in the DP
> property itself. Please correct me if I'm wrong, but I think that we can
> drop it for all the practical purpo
Re: [Freedreno] [PATCH v12 2/5] dt-bindings: msm/dp: add data-lanes and link-frequencies property
On Fri, 16 Dec 2022 at 00:57, Doug Anderson wrote:
>
> Hi,
>
> On Thu, Dec 15, 2022 at 1:12 PM Dmitry Baryshkov
> wrote:
> >
> > On 15/12/2022 02:38, Stephen Boyd wrote:
> > > Quoting Kuogee Hsieh (2022-12-14 14:56:23)
> > >>
> > >> On 12/13/2022 3:06 PM, Stephen Boyd wrote:
> > >>> Quoting Kuogee Hsieh (2022-12-13 13:44:05)
> > Add both data-lanes and link-frequencies property into endpoint
> > >>> Why do we care? Please tell us why it's important.
> > >
> > > Any response?
> > >
> > @@ -193,6 +217,8 @@ examples:
> > reg = <1>;
> > endpoint {
> > remote-endpoint = <&typec>;
> > +data-lanes = <0 1>;
> > +link-frequencies = /bits/ 64 <162000
> > 27 54 81>;
> > };
> > >>> So far we haven't used the output port on the DP controller in DT.
> > >>>
> > >>> I'm still not clear on what we should do in general for DP because
> > >>> there's a PHY that actually controls a lane count and lane mapping. In
> > >>> my mental model of the SoC, this DP controller's output port is
> > >>> connected to the DP PHY, which then sends the DP lanes out of the SoC to
> > >>> the next downstream device (i.e. a DP connector or type-c muxer). Having
> > >>> a remote-endpoint property with a phandle to typec doesn't fit my mental
> > >>> model. I'd expect it to be the typec PHY.
> > >> ack
> > >>>
> > >>> That brings up the question: when we have 2 lanes vs. 4 lanes will we
> > >>> duplicate the data-lanes property in the PHY binding? I suspect we'll
> > >>> have to. Hopefully that sort of duplication is OK?
> > >> Current we have limitation by reserve 2 data lanes for usb2, i am not
> > >> sure duplication to 4 lanes will work automatically.
> > >>>
> > >>> Similarly, we may have a redriver that limits the link-frequencies
> > >>> property further (e.g. only support <= 2.7GHz). Having multiple
> > >>> link-frequencies along the graph is OK, right? And isn't the
> > >>> link-frequencies property known here by fact that the DP controller
> > >>> tells us which SoC this controller is for, and thus we already know the
> > >>> supported link frequencies?
> > >>>
> > >>> Finally, I wonder if we should put any of this in the DP controller's
> > >>> output endpoint, or if we can put these sorts of properties in the DP
> > >>> PHY binding directly? Can't we do that and then when the DP controller
> > >>> tries to set 4 lanes, the PHY immediately fails the call and the link
> > >>> training algorithm does its thing and tries fewer lanes? And similarly,
> > >>> if link-frequencies were in the PHY's binding, the PHY could fail to set
> > >>> those frequencies during link training, returning an error to the DP
> > >>> controller, letting the training move on to a lower frequency. If we did
> > >>> that this patch series would largely be about modifying the PHY binding,
> > >>> updating the PHY driver to enforce constraints, and handling errors
> > >>> during link training in the DP controller (which may already be done? I
> > >>> didn't check).
> > >>
> > >>
> > >> phy/pll have different configuration base on link lanes and rate.
> > >>
> > >> it has to be set up before link training can start.
> > >>
> > >> Once link training start, then there are no any interactions between
> > >> controller and phy during link training session.
> > >
> > > What do you mean? The DP controller calls phy_configure() and changes
> > > the link rate. The return value from phy_configure() should be checked
> > > and link training should skip link rates that aren't supported and/or
> > > number of lanes that aren't supported.
> >
> > I'd toss another coin into the argument. We have previously discussed
> > using the link-frequencies property in the context of limiting link
> > speeds for the DSI. There we have both hardware (SoC) limitations and
> > the board limitations as in some cases the DSI lanes can not sustain
> > some high rate. I still hope for these patches to materialize at some point.
> >
> > For the DP this is more or less the same story. We have the hardware
> > (SoC, PHY, etc) limitations, but also we have the board/device
> > limitations. For example some of the board might not be able to support
> > HBR3 e.g. because of the PCB design. And while it might be logical to
> > also add the 'max bit rate' support to the eDP & combo PHYs, it
> > definitely makes sense to be able to limit the rate on the DP <->
> > `something' link.
> >
> > Now, for all the practical purposes this `something' for the DP is the
> > DP connector, the eDP panel or the USB-C mux (with the possible
> > redrivers in the middle).
> >
> > Thus I'd support Kuogee's proposal to have link-frequencies in the DP's
> > outbound endpoint. This is the link which will be driven by the data
> > stream from the Linux point of view. The PHY is linked through the
> > 'phys' property, but it doesn't pa
[Freedreno] [PATCH v4 0/2] do not complete dp_aux_cmd_fifo_tx() if irq is not for aux transfer
ignore spuriors isr at dp_aux_isr() to fixed eDP edid read failed Kuogee Hsieh (2): drm/msm/dp: do not complete dp_aux_cmd_fifo_tx() if irq is not for aux transfer drm/msm/dp: enhance dp controller isr drivers/gpu/drm/msm/dp/dp_aux.c | 98 + 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, 89 insertions(+), 42 deletions(-) -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
[Freedreno] [PATCH v4 2/2] drm/msm/dp: enhance dp controller isr
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
Suggested-by: Stephen Boyd
---
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;
- }
+ irqreturn_t ret = IRQ_NONE;
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)
[Freedreno] [PATCH v4 1/2] drm/msm/dp: do not complete dp_aux_cmd_fifo_tx() if irq is not for aux transfer
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.
Current there is a bug report regrading eDP edid corruption happen during
system booting up. After lengthy debugging to found that that VIDEO_READY
interrupt was continuously firing during system booting up which cause
dp_aux_isr() to complete dp_aux_cmd_fifo_tx() prematurely to retrieve data
from aux hardware buffer which is not yet contains complete data transfer
from sink. This cause edid corruption.
Follows are the signature at kernel logs when problem happen,
EDID has corrupt header
panel-simple-dp-aux aux-aea.edp: Couldn't identify panel via EDID
panel-simple-dp-aux aux-aea.edp: error -EIO: Couldn't detect panel nor find
a fallback
Changes in v2:
-- do complete if (ret == IRQ_HANDLED) ay dp-aux_isr()
-- add more commit text
Changes in v3:
-- add Stephen suggested
-- dp_aux_isr() return IRQ_XXX back to caller
-- dp_ctrl_isr() return IRQ_XXX back to caller
Changes in v4:
-- split into two patches
Fixes: c943b4948b58 ("drm/msm/dp: add displayPort driver support")
Signed-off-by: Kuogee Hsieh
Tested-by: Douglas Anderson
Reviewed-by: Abhinav Kumar
---
drivers/gpu/drm/msm/dp/dp_aux.c | 4
1 file changed, 4 insertions(+)
diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c b/drivers/gpu/drm/msm/dp/dp_aux.c
index d030a93..cc3efed 100644
--- a/drivers/gpu/drm/msm/dp/dp_aux.c
+++ b/drivers/gpu/drm/msm/dp/dp_aux.c
@@ -423,6 +423,10 @@ void dp_aux_isr(struct drm_dp_aux *dp_aux)
isr = dp_catalog_aux_get_irq(aux->catalog);
+ /* no interrupts pending, return immediately */
+ if (!isr)
+ return;
+
if (!aux->cmd_busy)
return;
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
Re: [Freedreno] [PATCH v2] drm/msm/dp: do not complete dp_aux_cmd_fifo_tx() if irq is not for aux transfer
On 12/15/2022 1:15 PM, Dmitry Baryshkov wrote: On 15/12/2022 22:10, Stephen Boyd wrote: Quoting Dmitry Baryshkov (2022-12-15 10:46:42) On 15/12/2022 20:32, Kuogee Hsieh wrote: if (!aux->cmd_busy) return; 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); Can you just move the complete() into the individual handling functions? Then you won't have to return the error code from dp_aux_*_handler() at all. You can check `isr' in that function and call complete if there was any error. I'd prefer we apply my patch and pass the irqreturn_t variable to the caller of this function so spurious irqs are shutdown. Should I send it as a proper patch? I'm for handling the spurious IRQs in a proper way. However I believe that it's not related to the issue Kuogee is trying to fix. Thus I think we should have two separate patches: one fixing the EDID corruption issue (for which the proper fix is !isr check, IIUC) and the irqreturn_t. And for the irqreturn_t it might be beneficial to move complete() call to the dp_aux_foo_handler(). Or might be not. That would depend on the patch itself. Ack.
Re: [Freedreno] [PATCH v2] drm/msm/dp: do not complete dp_aux_cmd_fifo_tx() if irq is not for aux transfer
On 12/15/2022 1:15 PM, Dmitry Baryshkov wrote: On 15/12/2022 22:10, Stephen Boyd wrote: Quoting Dmitry Baryshkov (2022-12-15 10:46:42) On 15/12/2022 20:32, Kuogee Hsieh wrote: if (!aux->cmd_busy) return; 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); Can you just move the complete() into the individual handling functions? Then you won't have to return the error code from dp_aux_*_handler() at all. You can check `isr' in that function and call complete if there was any error. I'd prefer we apply my patch and pass the irqreturn_t variable to the caller of this function so spurious irqs are shutdown. Should I send it as a proper patch? I'm for handling the spurious IRQs in a proper way. However I believe that it's not related to the issue Kuogee is trying to fix. Thus I think we should have two separate patches: one fixing the EDID corruption issue (for which the proper fix is !isr check, IIUC) and the irqreturn_t. And for the irqreturn_t it might be beneficial to move complete() call to the dp_aux_foo_handler(). Or might be not. That would depend on the patch itself. ok, I will split this patch into two.
Re: [Freedreno] [PATCH v2] drm/msm/dp: do not complete dp_aux_cmd_fifo_tx() if irq is not for aux transfer
On 15/12/2022 22:10, Stephen Boyd wrote: Quoting Dmitry Baryshkov (2022-12-15 10:46:42) On 15/12/2022 20:32, Kuogee Hsieh wrote: if (!aux->cmd_busy) return; 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); Can you just move the complete() into the individual handling functions? Then you won't have to return the error code from dp_aux_*_handler() at all. You can check `isr' in that function and call complete if there was any error. I'd prefer we apply my patch and pass the irqreturn_t variable to the caller of this function so spurious irqs are shutdown. Should I send it as a proper patch? I'm for handling the spurious IRQs in a proper way. However I believe that it's not related to the issue Kuogee is trying to fix. Thus I think we should have two separate patches: one fixing the EDID corruption issue (for which the proper fix is !isr check, IIUC) and the irqreturn_t. And for the irqreturn_t it might be beneficial to move complete() call to the dp_aux_foo_handler(). Or might be not. That would depend on the patch itself. -- With best wishes Dmitry
Re: [Freedreno] [PATCH v12 2/5] dt-bindings: msm/dp: add data-lanes and link-frequencies property
On 15/12/2022 02:38, Stephen Boyd wrote:
Quoting Kuogee Hsieh (2022-12-14 14:56:23)
On 12/13/2022 3:06 PM, Stephen Boyd wrote:
Quoting Kuogee Hsieh (2022-12-13 13:44:05)
Add both data-lanes and link-frequencies property into endpoint
Why do we care? Please tell us why it's important.
Any response?
@@ -193,6 +217,8 @@ examples:
reg = <1>;
endpoint {
remote-endpoint = <&typec>;
+data-lanes = <0 1>;
+link-frequencies = /bits/ 64 <162000 27
54 81>;
};
So far we haven't used the output port on the DP controller in DT.
I'm still not clear on what we should do in general for DP because
there's a PHY that actually controls a lane count and lane mapping. In
my mental model of the SoC, this DP controller's output port is
connected to the DP PHY, which then sends the DP lanes out of the SoC to
the next downstream device (i.e. a DP connector or type-c muxer). Having
a remote-endpoint property with a phandle to typec doesn't fit my mental
model. I'd expect it to be the typec PHY.
ack
That brings up the question: when we have 2 lanes vs. 4 lanes will we
duplicate the data-lanes property in the PHY binding? I suspect we'll
have to. Hopefully that sort of duplication is OK?
Current we have limitation by reserve 2 data lanes for usb2, i am not
sure duplication to 4 lanes will work automatically.
Similarly, we may have a redriver that limits the link-frequencies
property further (e.g. only support <= 2.7GHz). Having multiple
link-frequencies along the graph is OK, right? And isn't the
link-frequencies property known here by fact that the DP controller
tells us which SoC this controller is for, and thus we already know the
supported link frequencies?
Finally, I wonder if we should put any of this in the DP controller's
output endpoint, or if we can put these sorts of properties in the DP
PHY binding directly? Can't we do that and then when the DP controller
tries to set 4 lanes, the PHY immediately fails the call and the link
training algorithm does its thing and tries fewer lanes? And similarly,
if link-frequencies were in the PHY's binding, the PHY could fail to set
those frequencies during link training, returning an error to the DP
controller, letting the training move on to a lower frequency. If we did
that this patch series would largely be about modifying the PHY binding,
updating the PHY driver to enforce constraints, and handling errors
during link training in the DP controller (which may already be done? I
didn't check).
phy/pll have different configuration base on link lanes and rate.
it has to be set up before link training can start.
Once link training start, then there are no any interactions between
controller and phy during link training session.
What do you mean? The DP controller calls phy_configure() and changes
the link rate. The return value from phy_configure() should be checked
and link training should skip link rates that aren't supported and/or
number of lanes that aren't supported.
I'd toss another coin into the argument. We have previously discussed
using the link-frequencies property in the context of limiting link
speeds for the DSI. There we have both hardware (SoC) limitations and
the board limitations as in some cases the DSI lanes can not sustain
some high rate. I still hope for these patches to materialize at some point.
For the DP this is more or less the same story. We have the hardware
(SoC, PHY, etc) limitations, but also we have the board/device
limitations. For example some of the board might not be able to support
HBR3 e.g. because of the PCB design. And while it might be logical to
also add the 'max bit rate' support to the eDP & combo PHYs, it
definitely makes sense to be able to limit the rate on the DP <->
`something' link.
Now, for all the practical purposes this `something' for the DP is the
DP connector, the eDP panel or the USB-C mux (with the possible
redrivers in the middle).
Thus I'd support Kuogee's proposal to have link-frequencies in the DP's
outbound endpoint. This is the link which will be driven by the data
stream from the Linux point of view. The PHY is linked through the
'phys' property, but it doesn't participate in the USB-C (or in the
connector/panel) graph.
Now let's discuss the data lanes. Currently we have them in the DP
property itself. Please correct me if I'm wrong, but I think that we can
drop it for all the practical purposes. Judging by the DP compat string
the driver can determine if it uses 2 lanes (eDP) or 4 lanes
(full-featured DP). In case of USB-C when the altmode dictates whether
to use 2 or 4 lanes, the TCPM (Type-C Port Manager) will negotiate the
mode and pin configuration, then inform the DP controller about the
selected amount of lanes. Then DP informs the PHY about the selection
(note, PHY doesn't have control at all in this scenario).
[Freedreno] [PATCH v3] drm/msm/dp: do not complete dp_aux_cmd_fifo_tx() if irq is not for aux transfer
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.
Current there is a bug report regrading eDP edid corruption happen during
system booting up. After lengthy debugging to found that VIDEO_READY
interrupt was continuously firing during system booting up which cause
dp_aux_isr() to complete dp_aux_cmd_fifo_tx() prematurely to retrieve data
from aux hardware buffer which is not yet contains complete data transfer
from sink. This cause edid corruption.
Follows are the signature at kernel logs when problem happen,
EDID has corrupt header
panel-simple-dp-aux aux-aea.edp: Couldn't identify panel via EDID
panel-simple-dp-aux aux-aea.edp: error -EIO: Couldn't detect panel nor find
a fallback
Changes in v2:
-- do complete if (ret == IRQ_HANDLED) ay dp-aux_isr()
-- add more commit text
Changes in v3:
-- add Stephen suggested
-- dp_aux_isr() return IRQ_XXX back to caller
-- dp_ctrl_isr() return IRQ_XXX back to caller
Fixes: c943b4948b58 ("drm/msm/dp: add displayPort driver support")
Signed-off-by: Kuogee Hsieh
Tested-by: Douglas Anderson
Reviewed-by: Abhinav Kumar
Suggested-by: Stephen Boyd
---
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| 13 +++--
drivers/gpu/drm/msm/dp/dp_ctrl.h| 2 +-
drivers/gpu/drm/msm/dp/dp_display.c | 16 +--
5 files changed, 90 insertions(+), 38 deletions(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c b/drivers/gpu/drm/msm/dp/dp_aux.c
index d030a93..efb0f6c 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
Re: [Freedreno] [PATCH v2] drm/msm/dp: do not complete dp_aux_cmd_fifo_tx() if irq is not for aux transfer
On 12/15/2022 10:46 AM, Dmitry Baryshkov wrote:
On 15/12/2022 20:32, Kuogee Hsieh 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.
Current there is a bug report regrading eDP edid corruption happen during
system booting up. After lengthy debugging to found that VIDEO_READY
interrupt was continuously firing during system booting up which cause
dp_aux_isr() to complete dp_aux_cmd_fifo_tx() prematurely to retrieve
data
from aux hardware buffer which is not yet contains complete data transfer
from sink. This cause edid corruption.
Follows are the signature at kernel logs when problem happen,
EDID has corrupt header
panel-simple-dp-aux aux-aea.edp: Couldn't identify panel via EDID
panel-simple-dp-aux aux-aea.edp: error -EIO: Couldn't detect panel
nor find a fallback
Changes in v2:
-- do complete if (ret == IRQ_HANDLED) ay dp-aux_isr()
-- add more commit text
Usually it's a single dash.
Fixes: c943b4948b58 ("drm/msm/dp: add displayPort driver support")
Signed-off-by: Kuogee Hsieh
There should be no empty lines between the tags.
Tested-by: Douglas Anderson
Reviewed-by: Abhinav Kumar
---
drivers/gpu/drm/msm/dp/dp_aux.c | 87
+
1 file changed, 63 insertions(+), 24 deletions(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c
b/drivers/gpu/drm/msm/dp/dp_aux.c
index d030a93..f31e5c1 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) {
+
Re: [Freedreno] [PATCH v2] drm/msm/dp: do not complete dp_aux_cmd_fifo_tx() if irq is not for aux transfer
On 12/15/2022 12:10 PM, Stephen Boyd wrote: Quoting Dmitry Baryshkov (2022-12-15 10:46:42) On 15/12/2022 20:32, Kuogee Hsieh wrote: if (!aux->cmd_busy) return; 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); Can you just move the complete() into the individual handling functions? Then you won't have to return the error code from dp_aux_*_handler() at all. You can check `isr' in that function and call complete if there was any error. I'd prefer we apply my patch and pass the irqreturn_t variable to the caller of this function so spurious irqs are shutdown. Should I send it as a proper patch? yes, please
Re: [Freedreno] [PATCH v12 2/5] dt-bindings: msm/dp: add data-lanes and link-frequencies property
Quoting Kuogee Hsieh (2022-12-15 09:08:04) > > On 12/14/2022 4:38 PM, Stephen Boyd wrote: > > Quoting Kuogee Hsieh (2022-12-14 14:56:23) > >> On 12/13/2022 3:06 PM, Stephen Boyd wrote: > >>> Quoting Kuogee Hsieh (2022-12-13 13:44:05) > > > >> Therefore I think add data-lanes and link-frequencies properties in the > >> DP PHY binding directly will not helps. > >> > > I didn't follow your logic. Sorry. > > Sorry, probably i did not understand your proposal clearly. > > 1) move both data-lanes and link-frequencies property from dp controller > endpoint to phy > > 2) phy_configure() return succeed if both data-lanes and link > frequencies are supported. otherwise return failed. > > is above two summary items correct? Yes. > > Currently phy_configure() is part of link training process and called > if link lanes or rate changes. > > however, since current phy_configure() implementation always return 0, > the return value is not checking. > > This proposal is new, can we discuss more detail at meeting and decide > to implement it or not. > > Meanwhile can we merge current implementation (both data-lanes and > link-frequqncies at dp controller end point) first? > I don't think we can merge this patch because it depends on a DT binding change. If the PHY approach works then I'd prefer we just go with that.
Re: [Freedreno] [PATCH v2] drm/msm/dp: do not complete dp_aux_cmd_fifo_tx() if irq is not for aux transfer
Quoting Dmitry Baryshkov (2022-12-15 10:46:42) > On 15/12/2022 20:32, Kuogee Hsieh wrote: > > if (!aux->cmd_busy) > > return; > > > > 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); > > Can you just move the complete() into the individual handling functions? > Then you won't have to return the error code from dp_aux_*_handler() at > all. You can check `isr' in that function and call complete if there was > any error. I'd prefer we apply my patch and pass the irqreturn_t variable to the caller of this function so spurious irqs are shutdown. Should I send it as a proper patch?
Re: [Freedreno] [PATCH v2] drm/msm/dp: do not complete dp_aux_cmd_fifo_tx() if irq is not for aux transfer
On 15/12/2022 20:32, Kuogee Hsieh 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.
Current there is a bug report regrading eDP edid corruption happen during
system booting up. After lengthy debugging to found that VIDEO_READY
interrupt was continuously firing during system booting up which cause
dp_aux_isr() to complete dp_aux_cmd_fifo_tx() prematurely to retrieve data
from aux hardware buffer which is not yet contains complete data transfer
from sink. This cause edid corruption.
Follows are the signature at kernel logs when problem happen,
EDID has corrupt header
panel-simple-dp-aux aux-aea.edp: Couldn't identify panel via EDID
panel-simple-dp-aux aux-aea.edp: error -EIO: Couldn't detect panel nor find
a fallback
Changes in v2:
-- do complete if (ret == IRQ_HANDLED) ay dp-aux_isr()
-- add more commit text
Usually it's a single dash.
Fixes: c943b4948b58 ("drm/msm/dp: add displayPort driver support")
Signed-off-by: Kuogee Hsieh
There should be no empty lines between the tags.
Tested-by: Douglas Anderson
Reviewed-by: Abhinav Kumar
---
drivers/gpu/drm/msm/dp/dp_aux.c | 87 +
1 file changed, 63 insertions(+), 24 deletions(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c b/drivers/gpu/drm/msm/dp/dp_aux.c
index d030a93..f31e5c1 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 &
[Freedreno] [PATCH v2] drm/msm/dp: do not complete dp_aux_cmd_fifo_tx() if irq is not for aux transfer
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.
Current there is a bug report regrading eDP edid corruption happen during
system booting up. After lengthy debugging to found that VIDEO_READY
interrupt was continuously firing during system booting up which cause
dp_aux_isr() to complete dp_aux_cmd_fifo_tx() prematurely to retrieve data
from aux hardware buffer which is not yet contains complete data transfer
from sink. This cause edid corruption.
Follows are the signature at kernel logs when problem happen,
EDID has corrupt header
panel-simple-dp-aux aux-aea.edp: Couldn't identify panel via EDID
panel-simple-dp-aux aux-aea.edp: error -EIO: Couldn't detect panel nor find
a fallback
Changes in v2:
-- do complete if (ret == IRQ_HANDLED) ay dp-aux_isr()
-- add more commit text
Fixes: c943b4948b58 ("drm/msm/dp: add displayPort driver support")
Signed-off-by: Kuogee Hsieh
Tested-by: Douglas Anderson
Reviewed-by: Abhinav Kumar
---
drivers/gpu/drm/msm/dp/dp_aux.c | 87 +
1 file changed, 63 insertions(+), 24 deletions(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c b/drivers/gpu/drm/msm/dp/dp_aux.c
index d030a93..f31e5c1 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;
+ }
+
+ i
Re: [Freedreno] [PATCH] drm/msm/dp: do not complete dp_aux_cmd_fifo_tx() if irq is not for aux transfer
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 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 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-aea.edp: Couldn't identify panel via EDID panel-simple-dp-aux aux-aea.edp: error -EIO: Couldn't detect panel nor find a fallback Signed-off-by: Kuogee Hsieh --- 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 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
Re: [Freedreno] [PATCH v12 2/5] dt-bindings: msm/dp: add data-lanes and link-frequencies property
On 12/14/2022 4:38 PM, Stephen Boyd wrote:
Quoting Kuogee Hsieh (2022-12-14 14:56:23)
On 12/13/2022 3:06 PM, Stephen Boyd wrote:
Quoting Kuogee Hsieh (2022-12-13 13:44:05)
Add both data-lanes and link-frequencies property into endpoint
Why do we care? Please tell us why it's important.
Any response?
yes, i did that at my local patch already.
@@ -193,6 +217,8 @@ examples:
reg = <1>;
endpoint {
remote-endpoint = <&typec>;
+data-lanes = <0 1>;
+link-frequencies = /bits/ 64 <162000 27
54 81>;
};
So far we haven't used the output port on the DP controller in DT.
I'm still not clear on what we should do in general for DP because
there's a PHY that actually controls a lane count and lane mapping. In
my mental model of the SoC, this DP controller's output port is
connected to the DP PHY, which then sends the DP lanes out of the SoC to
the next downstream device (i.e. a DP connector or type-c muxer). Having
a remote-endpoint property with a phandle to typec doesn't fit my mental
model. I'd expect it to be the typec PHY.
ack
That brings up the question: when we have 2 lanes vs. 4 lanes will we
duplicate the data-lanes property in the PHY binding? I suspect we'll
have to. Hopefully that sort of duplication is OK?
Current we have limitation by reserve 2 data lanes for usb2, i am not
sure duplication to 4 lanes will work automatically.
Similarly, we may have a redriver that limits the link-frequencies
property further (e.g. only support <= 2.7GHz). Having multiple
link-frequencies along the graph is OK, right? And isn't the
link-frequencies property known here by fact that the DP controller
tells us which SoC this controller is for, and thus we already know the
supported link frequencies?
Finally, I wonder if we should put any of this in the DP controller's
output endpoint, or if we can put these sorts of properties in the DP
PHY binding directly? Can't we do that and then when the DP controller
tries to set 4 lanes, the PHY immediately fails the call and the link
training algorithm does its thing and tries fewer lanes? And similarly,
if link-frequencies were in the PHY's binding, the PHY could fail to set
those frequencies during link training, returning an error to the DP
controller, letting the training move on to a lower frequency. If we did
that this patch series would largely be about modifying the PHY binding,
updating the PHY driver to enforce constraints, and handling errors
during link training in the DP controller (which may already be done? I
didn't check).
phy/pll have different configuration base on link lanes and rate.
it has to be set up before link training can start.
Once link training start, then there are no any interactions between
controller and phy during link training session.
What do you mean? The DP controller calls phy_configure() and changes
the link rate. The return value from phy_configure() should be checked
and link training should skip link rates that aren't supported and/or
number of lanes that aren't supported.
Link training only happen between dp controller and sink since link
status is reported by sink (read back from sink's dpcd register directly).
T achieve link symbol locked, link training will start from reduce link
rate until lowest rate, if it still failed, then it will reduce lanes
with highest rate and start training again.
it will repeat same process until lowest lane (one lane), if it still
failed, then it will give up and declare link training failed.
Yes, that describes the link training algorithm. I don't see why
phy_configure() return value can't be checked and either number of lanes
or link frequencies be checked. If only two lanes are supported, then
phy_configure() will fail for the 4 link rates and the algorithm will
reduce the number of lanes and go back to the highest rate. Then when
the highest rate isn't supported it will drop link rate until the link
rate is supported.
Therefore I think add data-lanes and link-frequencies properties in the
DP PHY binding directly will not helps.
I didn't follow your logic. Sorry.
Sorry, probably i did not understand your proposal clearly.
1) move both data-lanes and link-frequencies property from dp controller
endpoint to phy
2) phy_configure() return succeed if both data-lanes and link
frequencies are supported. otherwise return failed.
is above two summary items correct?
Currently phy_configure() is part of link training process and called
if link lanes or rate changes.
however, since current phy_configure() implementation always return 0,
the return value is not checking.
This proposal is new, can we discuss more detail at meeting and decide
to implement it or not.
Meanwhile can we merge current implementation (both data-lanes and
link-frequqncies at dp controller end point) first?
[Freedreno] [PATCH 5/5] drm/msm/a6xx: Use genpd notifier to ensure cx-gdsc collapse
As per the recommended recovery sequence of adreno gpu, cx gdsc should
collapse at hardware before it is turned back ON. This helps to clear
out the stale states in hardware before it is reinitialized. Use the
genpd notifier along with the newly introduced
dev_pm_genpd_synced_poweroff() api to ensure that cx gdsc has collapsed
before we turn it back ON.
Signed-off-by: Akhil P Oommen
---
drivers/gpu/drm/msm/adreno/a6xx_gmu.c | 15 +++
drivers/gpu/drm/msm/adreno/a6xx_gmu.h | 6 ++
drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 11 +++
3 files changed, 32 insertions(+)
diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
index 1580d0090f35..c03830957c26 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
@@ -1507,6 +1507,17 @@ void a6xx_gmu_remove(struct a6xx_gpu *a6xx_gpu)
gmu->initialized = false;
}
+static int cxpd_notifier_cb(struct notifier_block *nb,
+ unsigned long action, void *data)
+{
+ struct a6xx_gmu *gmu = container_of(nb, struct a6xx_gmu, pd_nb);
+
+ if (action == GENPD_NOTIFY_OFF)
+ complete_all(&gmu->pd_gate);
+
+ return 0;
+}
+
int a6xx_gmu_init(struct a6xx_gpu *a6xx_gpu, struct device_node *node)
{
struct adreno_gpu *adreno_gpu = &a6xx_gpu->base;
@@ -1640,6 +1651,10 @@ int a6xx_gmu_init(struct a6xx_gpu *a6xx_gpu, struct
device_node *node)
goto detach_cxpd;
}
+ init_completion(&gmu->pd_gate);
+ complete_all(&gmu->pd_gate);
+ gmu->pd_nb.notifier_call = cxpd_notifier_cb;
+
/*
* Get a link to the GX power domain to reset the GPU in case of GMU
* crash
diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.h
b/drivers/gpu/drm/msm/adreno/a6xx_gmu.h
index 5a42dd4dd31f..0bc3eb443fec 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.h
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.h
@@ -4,8 +4,10 @@
#ifndef _A6XX_GMU_H_
#define _A6XX_GMU_H_
+#include
#include
#include
+#include
#include "msm_drv.h"
#include "a6xx_hfi.h"
@@ -90,6 +92,10 @@ struct a6xx_gmu {
bool initialized;
bool hung;
bool legacy; /* a618 or a630 */
+
+ /* For power domain callback */
+ struct notifier_block pd_nb;
+ struct completion pd_gate;
};
static inline u32 gmu_read(struct a6xx_gmu *gmu, u32 offset)
diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
index 4b16e75dfa50..dd618b099110 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
@@ -10,6 +10,7 @@
#include
#include
+#include
#include
#define GPU_PAS_ID 13
@@ -1258,6 +1259,7 @@ static void a6xx_recover(struct msm_gpu *gpu)
{
struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
struct a6xx_gpu *a6xx_gpu = to_a6xx_gpu(adreno_gpu);
+ struct a6xx_gmu *gmu = &a6xx_gpu->gmu;
int i, active_submits;
adreno_dump_info(gpu);
@@ -1290,6 +1292,10 @@ static void a6xx_recover(struct msm_gpu *gpu)
*/
gpu->active_submits = 0;
+ reinit_completion(&gmu->pd_gate);
+ dev_pm_genpd_add_notifier(gmu->cxpd, &gmu->pd_nb);
+ dev_pm_genpd_synced_poweroff(gmu->cxpd);
+
/* Drop the rpm refcount from active submits */
if (active_submits)
pm_runtime_put(&gpu->pdev->dev);
@@ -1297,6 +1303,11 @@ static void a6xx_recover(struct msm_gpu *gpu)
/* And the final one from recover worker */
pm_runtime_put_sync(&gpu->pdev->dev);
+ if (!wait_for_completion_timeout(&gmu->pd_gate, msecs_to_jiffies(1000)))
+ DRM_DEV_ERROR(&gpu->pdev->dev, "cx gdsc didn't collapse\n");
+
+ dev_pm_genpd_remove_notifier(gmu->cxpd);
+
pm_runtime_use_autosuspend(&gpu->pdev->dev);
if (active_submits)
--
2.7.4
[Freedreno] [PATCH 3/5] drm/msm/a6xx: Vote for cx gdsc from gpu driver
When a device has multiple power domains, dev->power_domain is left
empty during probe. That didn't cause any issue so far because we are
freeloading on smmu driver's vote on cx gdsc. Instead of that, create
a device_link between cx genpd device and gmu device to keep a vote from
gpu driver.
Before this patch:
localhost ~ # cat /sys/kernel/debug/pm_genpd/pm_genpd_summary
gx_gdsc on 0
/devices/genpd:1:3d6a000.gmuactive 0
cx_gdsc on 0
/devices/platform/soc@0/3da.iommu active 0
After this patch:
localhost ~ # cat /sys/kernel/debug/pm_genpd/pm_genpd_summary
gx_gdsc on 0
/devices/genpd:1:3d6a000.gmuactive 0
cx_gdsc on 0
/devices/platform/soc@0/3da.iommu active 0
/devices/genpd:0:3d6a000.gmuactive 0
Signed-off-by: Akhil P Oommen
---
drivers/gpu/drm/msm/adreno/a6xx_gmu.c | 31 +++
drivers/gpu/drm/msm/adreno/a6xx_gmu.h | 1 +
2 files changed, 28 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
index 6484b97c5344..1580d0090f35 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
@@ -1479,6 +1479,12 @@ void a6xx_gmu_remove(struct a6xx_gpu *a6xx_gpu)
pm_runtime_force_suspend(gmu->dev);
+ /*
+* Since cxpd is a virt device, the devlink with gmu-dev will be removed
+* automatically when we do detach
+*/
+ dev_pm_domain_detach(gmu->cxpd, false);
+
if (!IS_ERR_OR_NULL(gmu->gxpd)) {
pm_runtime_disable(gmu->gxpd);
dev_pm_domain_detach(gmu->gxpd, false);
@@ -1605,8 +1611,10 @@ int a6xx_gmu_init(struct a6xx_gpu *a6xx_gpu, struct
device_node *node)
if (adreno_is_a650_family(adreno_gpu)) {
gmu->rscc = a6xx_gmu_get_mmio(pdev, "rscc");
- if (IS_ERR(gmu->rscc))
+ if (IS_ERR(gmu->rscc)) {
+ ret = -ENODEV;
goto err_mmio;
+ }
} else {
gmu->rscc = gmu->mmio + 0x23000;
}
@@ -1615,8 +1623,22 @@ int a6xx_gmu_init(struct a6xx_gpu *a6xx_gpu, struct
device_node *node)
gmu->hfi_irq = a6xx_gmu_get_irq(gmu, pdev, "hfi", a6xx_hfi_irq);
gmu->gmu_irq = a6xx_gmu_get_irq(gmu, pdev, "gmu", a6xx_gmu_irq);
- if (gmu->hfi_irq < 0 || gmu->gmu_irq < 0)
+ if (gmu->hfi_irq < 0 || gmu->gmu_irq < 0) {
+ ret = -ENODEV;
+ goto err_mmio;
+ }
+
+ gmu->cxpd = dev_pm_domain_attach_by_name(gmu->dev, "cx");
+ if (IS_ERR(gmu->cxpd)) {
+ ret = PTR_ERR(gmu->cxpd);
goto err_mmio;
+ }
+
+ if (!device_link_add(gmu->dev, gmu->cxpd,
+ DL_FLAG_PM_RUNTIME)) {
+ ret = -ENODEV;
+ goto detach_cxpd;
+ }
/*
* Get a link to the GX power domain to reset the GPU in case of GMU
@@ -1634,6 +1656,9 @@ int a6xx_gmu_init(struct a6xx_gpu *a6xx_gpu, struct
device_node *node)
return 0;
+detach_cxpd:
+ dev_pm_domain_detach(gmu->cxpd, false);
+
err_mmio:
iounmap(gmu->mmio);
if (platform_get_resource_byname(pdev, IORESOURCE_MEM, "rscc"))
@@ -1641,8 +1666,6 @@ int a6xx_gmu_init(struct a6xx_gpu *a6xx_gpu, struct
device_node *node)
free_irq(gmu->gmu_irq, gmu);
free_irq(gmu->hfi_irq, gmu);
- ret = -ENODEV;
-
err_memory:
a6xx_gmu_memory_free(gmu);
err_put_device:
diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.h
b/drivers/gpu/drm/msm/adreno/a6xx_gmu.h
index e034935b3986..5a42dd4dd31f 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.h
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.h
@@ -56,6 +56,7 @@ struct a6xx_gmu {
int gmu_irq;
struct device *gxpd;
+ struct device *cxpd;
int idle_level;
--
2.7.4
[Freedreno] [PATCH 4/5] drm/msm/a6xx: Remove cx gdsc polling using 'reset'
Remove the unused 'reset' interface which was supposed to help to ensure
that cx gdsc has collapsed during gpu recovery. This is was not enabled
so far due to missing gpucc driver support. Similar functionality using
genpd framework will be implemented in the upcoming patch.
Signed-off-by: Akhil P Oommen
---
drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 4
drivers/gpu/drm/msm/msm_gpu.c | 4
drivers/gpu/drm/msm/msm_gpu.h | 4
3 files changed, 12 deletions(-)
diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
index 36c8fb699b56..4b16e75dfa50 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
@@ -10,7 +10,6 @@
#include
#include
-#include
#include
#define GPU_PAS_ID 13
@@ -1298,9 +1297,6 @@ static void a6xx_recover(struct msm_gpu *gpu)
/* And the final one from recover worker */
pm_runtime_put_sync(&gpu->pdev->dev);
- /* Call into gpucc driver to poll for cx gdsc collapse */
- reset_control_reset(gpu->cx_collapse);
-
pm_runtime_use_autosuspend(&gpu->pdev->dev);
if (active_submits)
diff --git a/drivers/gpu/drm/msm/msm_gpu.c b/drivers/gpu/drm/msm/msm_gpu.c
index 30ed45af76ad..97e1319d4577 100644
--- a/drivers/gpu/drm/msm/msm_gpu.c
+++ b/drivers/gpu/drm/msm/msm_gpu.c
@@ -16,7 +16,6 @@
#include
#include
#include
-#include
#include
/*
@@ -933,9 +932,6 @@ int msm_gpu_init(struct drm_device *drm, struct
platform_device *pdev,
if (IS_ERR(gpu->gpu_cx))
gpu->gpu_cx = NULL;
- gpu->cx_collapse = devm_reset_control_get_optional_exclusive(&pdev->dev,
- "cx_collapse");
-
gpu->pdev = pdev;
platform_set_drvdata(pdev, &gpu->adreno_smmu);
diff --git a/drivers/gpu/drm/msm/msm_gpu.h b/drivers/gpu/drm/msm/msm_gpu.h
index 651786bc55e5..fa9e34d02c91 100644
--- a/drivers/gpu/drm/msm/msm_gpu.h
+++ b/drivers/gpu/drm/msm/msm_gpu.h
@@ -13,7 +13,6 @@
#include
#include
#include
-#include
#include "msm_drv.h"
#include "msm_fence.h"
@@ -282,9 +281,6 @@ struct msm_gpu {
bool hw_apriv;
struct thermal_cooling_device *cooling;
-
- /* To poll for cx gdsc collapse during gpu recovery */
- struct reset_control *cx_collapse;
};
static inline struct msm_gpu *dev_to_gpu(struct device *dev)
--
2.7.4
[Freedreno] [PATCH 2/5] clk: qcom: gdsc: Support 'synced_poweroff' genpd flag
Add support for the newly added 'synced_poweroff' genpd flag. This allows
some clients (like adreno gpu driver) to request gdsc driver to ensure
a votable gdsc (like gpucc cx gdsc) has collapsed at hardware.
Signed-off-by: Akhil P Oommen
---
drivers/clk/qcom/gdsc.c | 11 ++-
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
index 9e4d6ce891aa..575019ba4768 100644
--- a/drivers/clk/qcom/gdsc.c
+++ b/drivers/clk/qcom/gdsc.c
@@ -136,7 +136,8 @@ static int gdsc_update_collapse_bit(struct gdsc *sc, bool
val)
return 0;
}
-static int gdsc_toggle_logic(struct gdsc *sc, enum gdsc_status status)
+static int gdsc_toggle_logic(struct gdsc *sc, enum gdsc_status status,
+ bool force_sync)
{
int ret;
@@ -149,7 +150,7 @@ static int gdsc_toggle_logic(struct gdsc *sc, enum
gdsc_status status)
ret = gdsc_update_collapse_bit(sc, status == GDSC_OFF);
/* If disabling votable gdscs, don't poll on status */
- if ((sc->flags & VOTABLE) && status == GDSC_OFF) {
+ if ((sc->flags & VOTABLE) && status == GDSC_OFF && !force_sync) {
/*
* Add a short delay here to ensure that an enable
* right after it was disabled does not put it in an
@@ -275,7 +276,7 @@ static int gdsc_enable(struct generic_pm_domain *domain)
gdsc_deassert_clamp_io(sc);
}
- ret = gdsc_toggle_logic(sc, GDSC_ON);
+ ret = gdsc_toggle_logic(sc, GDSC_ON, false);
if (ret)
return ret;
@@ -352,7 +353,7 @@ static int gdsc_disable(struct generic_pm_domain *domain)
if (sc->pwrsts == PWRSTS_RET_ON)
return 0;
- ret = gdsc_toggle_logic(sc, GDSC_OFF);
+ ret = gdsc_toggle_logic(sc, GDSC_OFF, domain->synced_poweroff);
if (ret)
return ret;
@@ -392,7 +393,7 @@ static int gdsc_init(struct gdsc *sc)
/* Force gdsc ON if only ON state is supported */
if (sc->pwrsts == PWRSTS_ON) {
- ret = gdsc_toggle_logic(sc, GDSC_ON);
+ ret = gdsc_toggle_logic(sc, GDSC_ON, false);
if (ret)
return ret;
}
--
2.7.4
[Freedreno] [PATCH 1/5] PM: domains: Allow a genpd consumer to require a synced power off
From: Ulf Hansson
Some genpd providers doesn't ensure that it has turned off at hardware.
This is fine until the consumer really requires during some special
scenarios that the power domain collapse at hardware before it is
turned ON again.
An example is the reset sequence of Adreno GPU which requires that the
'gpucc cx gdsc' power domain should move to OFF state in hardware at
least once before turning in ON again to clear the internal state.
Signed-off-by: Ulf Hansson
Signed-off-by: Akhil P Oommen
---
@Ulf, I took the liberty to cleanup and post your patch.
drivers/base/power/domain.c | 23 +++
include/linux/pm_domain.h | 5 +
2 files changed, 28 insertions(+)
diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 967bcf9d415e..4fa624218967 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -519,6 +519,28 @@ ktime_t dev_pm_genpd_get_next_hrtimer(struct device *dev)
}
EXPORT_SYMBOL_GPL(dev_pm_genpd_get_next_hrtimer);
+/*
+ * dev_pm_genpd_synced_poweroff - Next power off should be synchronous
+ *
+ * @dev: A device that is attached to the genpd.
+ *
+ * Allows a consumer of the genpd to notify the provider that the next power
off
+ * should be synchronous.
+ */
+void dev_pm_genpd_synced_poweroff(struct device *dev)
+{
+ struct generic_pm_domain *genpd;
+
+ genpd = dev_to_genpd_safe(dev);
+ if (!genpd)
+ return;
+
+ genpd_lock(genpd);
+ genpd->synced_poweroff = true;
+ genpd_unlock(genpd);
+}
+EXPORT_SYMBOL_GPL(dev_pm_genpd_synced_poweroff);
+
static int _genpd_power_on(struct generic_pm_domain *genpd, bool timed)
{
unsigned int state_idx = genpd->state_idx;
@@ -562,6 +584,7 @@ static int _genpd_power_on(struct generic_pm_domain *genpd,
bool timed)
out:
raw_notifier_call_chain(&genpd->power_notifiers, GENPD_NOTIFY_ON, NULL);
+ genpd->synced_poweroff = false;
return 0;
err:
raw_notifier_call_chain(&genpd->power_notifiers, GENPD_NOTIFY_OFF,
diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
index 1cd41bdf73cf..f776fb93eaa0 100644
--- a/include/linux/pm_domain.h
+++ b/include/linux/pm_domain.h
@@ -136,6 +136,7 @@ struct generic_pm_domain {
unsigned int prepared_count;/* Suspend counter of prepared devices
*/
unsigned int performance_state; /* Aggregated max performance state */
cpumask_var_t cpus; /* A cpumask of the attached CPUs */
+ bool synced_poweroff; /* A consumer needs a synced poweroff */
int (*power_off)(struct generic_pm_domain *domain);
int (*power_on)(struct generic_pm_domain *domain);
struct raw_notifier_head power_notifiers; /* Power on/off notifiers */
@@ -235,6 +236,7 @@ int dev_pm_genpd_add_notifier(struct device *dev, struct
notifier_block *nb);
int dev_pm_genpd_remove_notifier(struct device *dev);
void dev_pm_genpd_set_next_wakeup(struct device *dev, ktime_t next);
ktime_t dev_pm_genpd_get_next_hrtimer(struct device *dev);
+void dev_pm_genpd_synced_poweroff(struct device *dev);
extern struct dev_power_governor simple_qos_governor;
extern struct dev_power_governor pm_domain_always_on_gov;
@@ -300,6 +302,9 @@ static inline ktime_t dev_pm_genpd_get_next_hrtimer(struct
device *dev)
{
return KTIME_MAX;
}
+static inline void dev_pm_genpd_synced_poweroff(struct device *dev)
+{ }
+
#define simple_qos_governor(*(struct dev_power_governor *)(NULL))
#define pm_domain_always_on_gov(*(struct dev_power_governor
*)(NULL))
#endif
--
2.7.4
[Freedreno] [PATCH 0/5] Improve GPU reset sequence for Adreno GPU
This is a rework of [1] using genpd instead of 'reset' framework. As per the recommended reset sequence of Adreno gpu, we should ensure that gpucc-cx-gdsc has collapsed at hardware to reset gpu's internal hardware states. Because this gdsc is implemented as 'votable', gdsc driver doesn't poll and wait until its hw status says OFF. So use the newly introduced genpd api (dev_pm_genpd_synced_poweroff()) to provide a hint to the gdsc driver to poll for the hw status and use genpd notifier to wait from adreno gpu driver until gdsc is turned OFF. This series is rebased on top of linux-next (20221215) since the changes span multiple drivers. [1] https://patchwork.freedesktop.org/series/107507/ Akhil P Oommen (4): clk: qcom: gdsc: Support 'synced_poweroff' genpd flag drm/msm/a6xx: Vote for cx gdsc from gpu driver drm/msm/a6xx: Remove cx gdsc polling using 'reset' drm/msm/a6xx: Use genpd notifier to ensure cx-gdsc collapse Ulf Hansson (1): PM: domains: Allow a genpd consumer to require a synced power off drivers/base/power/domain.c | 23 ++ drivers/clk/qcom/gdsc.c | 11 + drivers/gpu/drm/msm/adreno/a6xx_gmu.c | 46 --- drivers/gpu/drm/msm/adreno/a6xx_gmu.h | 7 ++ drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 13 +++--- drivers/gpu/drm/msm/msm_gpu.c | 4 --- drivers/gpu/drm/msm/msm_gpu.h | 4 --- include/linux/pm_domain.h | 5 8 files changed, 93 insertions(+), 20 deletions(-) -- 2.7.4
