LinuxLists.cc - [PATCH 1/1] drm/bridge: analogix

2023-02-08 03:16:15

Subject: [PATCH 1/1] drm/bridge: analogix_dp: add a quirk for Bob panel

From: zain wang <[email protected]>

Some panels' DP_PSR_STATUS (DPCD 0x2008) may be unstable when we
enable psr. If we get the unexpected psr state, We need try the
debounce to ensure the panel was in PSR

Signed-off-by: zain wang <[email protected]>
Signed-off-by: Chris Zhong <[email protected]>
Commit-Ready: Kristian H. Kristensen <[email protected]>
Tested-by: Kristian H. Kristensen <[email protected]>
Reviewed-by: Kristian H. Kristensen <[email protected]>
Tested-by: Kencp huang <[email protected]>
Signed-off-by: Kencp huang <[email protected]>
---
.../gpu/drm/bridge/analogix/analogix_dp_reg.c | 71 +++++++++++--------
1 file changed, 42 insertions(+), 29 deletions(-)

diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
index 6a4f20fccf84..7b6e3f8f85b0 100644
--- a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
+++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
@@ -935,25 +935,54 @@ void analogix_dp_enable_psr_crc(struct analogix_dp_device *dp)
writel(PSR_VID_CRC_ENABLE, dp->reg_base + ANALOGIX_DP_CRC_CON);
}

-static ssize_t analogix_dp_get_psr_status(struct analogix_dp_device *dp)
+static int analogix_dp_get_psr_status(struct analogix_dp_device *dp,
+ int status)
{
ssize_t val;
- u8 status;
+ u8 reg, store = 0;
+ int cnt = 0;
+
+ /* About 3ms for a loop */
+ while (cnt < 100) {
+ /* Read operation would takes 900us */
+ val = drm_dp_dpcd_readb(&dp->aux, DP_PSR_STATUS, &reg);
+ if (val < 0) {
+ dev_err(dp->dev, "PSR_STATUS read failed ret=%zd", val);
+ return val;
+ }
+
+ /*
+ * Ensure the PSR_STATE should go to DP_PSR_SINK_ACTIVE_RFB
+ * from DP_PSR_SINK_ACTIVE_SINK_SYNCED or
+ * DP_PSR_SINK_ACTIVE_SRC_SYNCED.
+ * Otherwise, if we get DP_PSR_SINK_ACTIVE_RFB twice in
+ * succession, it show the Panel is stable PSR enabled state.
+ */
+ if (status == DP_PSR_SINK_ACTIVE_RFB) {
+ if ((reg == DP_PSR_SINK_ACTIVE_RFB) &&
+ ((store == DP_PSR_SINK_ACTIVE_SINK_SYNCED) ||
+ (store == DP_PSR_SINK_ACTIVE_SRC_SYNCED) ||
+ (store == DP_PSR_SINK_ACTIVE_RFB)))
+ return 0;
+ else
+ store = reg;
+ } else {
+ if ((reg == DP_PSR_SINK_INACTIVE) ||
+ (reg == DP_PSR_SINK_ACTIVE_RESYNC))
+ return 0;
+ }

- val = drm_dp_dpcd_readb(&dp->aux, DP_PSR_STATUS, &status);
- if (val < 0) {
- dev_err(dp->dev, "PSR_STATUS read failed ret=%zd", val);
- return val;
+ usleep_range(2100, 2200);
+ cnt++;
}
- return status;
+
+ return -ETIMEDOUT;
}

int analogix_dp_send_psr_spd(struct analogix_dp_device *dp,
struct dp_sdp *vsc, bool blocking)
{
unsigned int val;
- int ret;
- ssize_t psr_status;

/* don't send info frame */
val = readl(dp->reg_base + ANALOGIX_DP_PKT_SEND_CTL);
@@ -998,26 +1027,10 @@ int analogix_dp_send_psr_spd(struct analogix_dp_device *dp,
if (!blocking)
return 0;

- /*
- * db[1]!=0: entering PSR, wait for fully active remote frame buffer.
- * db[1]==0: exiting PSR, wait for either
- * (a) ACTIVE_RESYNC - the sink "must display the
- * incoming active frames from the Source device with no visible
- * glitches and/or artifacts", even though timings may still be
- * re-synchronizing; or
- * (b) INACTIVE - the transition is fully complete.
- */
- ret = readx_poll_timeout(analogix_dp_get_psr_status, dp, psr_status,
- psr_status >= 0 &&
- ((vsc->db[1] && psr_status == DP_PSR_SINK_ACTIVE_RFB) ||
- (!vsc->db[1] && (psr_status == DP_PSR_SINK_ACTIVE_RESYNC ||
- psr_status == DP_PSR_SINK_INACTIVE))),
- 1500, DP_TIMEOUT_PSR_LOOP_MS * 1000);
- if (ret) {
- dev_warn(dp->dev, "Failed to apply PSR %d\n", ret);
- return ret;
- }
- return 0;
+ if (vsc->db[1])
+ return analogix_dp_get_psr_status(dp, DP_PSR_SINK_ACTIVE_RFB);
+ else
+ return analogix_dp_get_psr_status(dp, 0);
}

ssize_t analogix_dp_transfer(struct analogix_dp_device *dp,
--
2.34.1

2023-02-08 04:44:41

by Kencp huang

[permalink] [raw]

Subject: [PATCH 1/1] drm/bridge: analogix_dp: add a quirk for Bob panel

2023-02-14 21:02:24

by Brian Norris

[permalink] [raw]

Subject: Re: [PATCH 1/1] drm/bridge: analogix_dp: add a quirk for Bob panel

Hi,

You seem to have sent this twice, perhaps to adjust the To/CC list. I
think I've picked the right one to reply to, but it's usually nice to
use a "v2" notation or otherwise put a comment somewhere in the email.

On Wed, Feb 08, 2023 at 12:44:06PM +0800, Kencp huang wrote:
> From: zain wang <[email protected]>
>
> Some panels' DP_PSR_STATUS (DPCD 0x2008) may be unstable when we
> enable psr. If we get the unexpected psr state, We need try the
> debounce to ensure the panel was in PSR
>
> Signed-off-by: zain wang <[email protected]>
> Signed-off-by: Chris Zhong <[email protected]>
> Commit-Ready: Kristian H. Kristensen <[email protected]>

'Commit-Ready' isn't something that makes sense for upstream Linux. The
other 'Tested-by' and 'Reviewed-by' *might* make sense to carry forward,
even though these were from the Chromium Gerrit instance, but they also
applied to a very old and different version of this patch, so probably
not.

I'd suggest starting over with only the Signed-off-by tags.

> Tested-by: Kristian H. Kristensen <[email protected]>
> Reviewed-by: Kristian H. Kristensen <[email protected]>
> Tested-by: Kencp huang <[email protected]>
> Signed-off-by: Kencp huang <[email protected]>
> ---
> .../gpu/drm/bridge/analogix/analogix_dp_reg.c | 71 +++++++++++--------
> 1 file changed, 42 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
> index 6a4f20fccf84..7b6e3f8f85b0 100644
> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
> @@ -935,25 +935,54 @@ void analogix_dp_enable_psr_crc(struct analogix_dp_device *dp)
> writel(PSR_VID_CRC_ENABLE, dp->reg_base + ANALOGIX_DP_CRC_CON);
> }
>
> -static ssize_t analogix_dp_get_psr_status(struct analogix_dp_device *dp)
> +static int analogix_dp_get_psr_status(struct analogix_dp_device *dp,

Probably could be called 'analogix_dp_wait_psr_status()', since it's no
longer just a "getter" function.

> + int status)

This would probably make more sense as a 'bool psr_active' or some
similar naming, since it doesn't really represent a "status" field now,
but more of a "am I entering or exiting PSR?" parameter.

> {
> ssize_t val;
> - u8 status;
> + u8 reg, store = 0;
> + int cnt = 0;
> +
> + /* About 3ms for a loop */

The commit description explains why you need this polling/debounce loop,
but it's good to document such artifacts in the code too, when they're
as strange as this one. Perhaps a short explanation about the "bouncing"
behavior of some panels here? "Some panels' DP_PSR_STATUS register is
unstable when entering PSR."

Also, I already had a (pre mailing list) question about why this doesn't
use readx_poll_timeout(), so I'll repeat for the record one reason not
to: it's difficult to handle the stateful debouncing aspect with that
macro, and keep the 'store' variable around.

> + while (cnt < 100) {
> + /* Read operation would takes 900us */
> + val = drm_dp_dpcd_readb(&dp->aux, DP_PSR_STATUS, &reg);
> + if (val < 0) {
> + dev_err(dp->dev, "PSR_STATUS read failed ret=%zd", val);
> + return val;
> + }
> +
> + /*
> + * Ensure the PSR_STATE should go to DP_PSR_SINK_ACTIVE_RFB
> + * from DP_PSR_SINK_ACTIVE_SINK_SYNCED or
> + * DP_PSR_SINK_ACTIVE_SRC_SYNCED.
> + * Otherwise, if we get DP_PSR_SINK_ACTIVE_RFB twice in
> + * succession, it show the Panel is stable PSR enabled state.
> + */
> + if (status == DP_PSR_SINK_ACTIVE_RFB) {
> + if ((reg == DP_PSR_SINK_ACTIVE_RFB) &&
> + ((store == DP_PSR_SINK_ACTIVE_SINK_SYNCED) ||
> + (store == DP_PSR_SINK_ACTIVE_SRC_SYNCED) ||
> + (store == DP_PSR_SINK_ACTIVE_RFB)))
> + return 0;
> + else
> + store = reg;
> + } else {

You dropped the ACTIVE_RESYNC and INACTIVE comments from below. Those
probably should move here.

With those fixed, I think this would be fine:

Reviewed-by: Brian Norris <[email protected]>

> + if ((reg == DP_PSR_SINK_INACTIVE) ||
> + (reg == DP_PSR_SINK_ACTIVE_RESYNC))
> + return 0;
> + }
>
> - val = drm_dp_dpcd_readb(&dp->aux, DP_PSR_STATUS, &status);
> - if (val < 0) {
> - dev_err(dp->dev, "PSR_STATUS read failed ret=%zd", val);
> - return val;
> + usleep_range(2100, 2200);
> + cnt++;
> }
> - return status;
> +
> + return -ETIMEDOUT;
> }
>
> int analogix_dp_send_psr_spd(struct analogix_dp_device *dp,
> struct dp_sdp *vsc, bool blocking)
> {
> unsigned int val;
> - int ret;
> - ssize_t psr_status;
>
> /* don't send info frame */
> val = readl(dp->reg_base + ANALOGIX_DP_PKT_SEND_CTL);
> @@ -998,26 +1027,10 @@ int analogix_dp_send_psr_spd(struct analogix_dp_device *dp,
> if (!blocking)
> return 0;
>
> - /*
> - * db[1]!=0: entering PSR, wait for fully active remote frame buffer.
> - * db[1]==0: exiting PSR, wait for either
> - * (a) ACTIVE_RESYNC - the sink "must display the
> - * incoming active frames from the Source device with no visible
> - * glitches and/or artifacts", even though timings may still be
> - * re-synchronizing; or
> - * (b) INACTIVE - the transition is fully complete.
> - */
> - ret = readx_poll_timeout(analogix_dp_get_psr_status, dp, psr_status,
> - psr_status >= 0 &&
> - ((vsc->db[1] && psr_status == DP_PSR_SINK_ACTIVE_RFB) ||
> - (!vsc->db[1] && (psr_status == DP_PSR_SINK_ACTIVE_RESYNC ||
> - psr_status == DP_PSR_SINK_INACTIVE))),
> - 1500, DP_TIMEOUT_PSR_LOOP_MS * 1000);
> - if (ret) {
> - dev_warn(dp->dev, "Failed to apply PSR %d\n", ret);
> - return ret;
> - }
> - return 0;
> + if (vsc->db[1])
> + return analogix_dp_get_psr_status(dp, DP_PSR_SINK_ACTIVE_RFB);
> + else
> + return analogix_dp_get_psr_status(dp, 0);
> }
>
> ssize_t analogix_dp_transfer(struct analogix_dp_device *dp,
> --
> 2.34.1
>