* [PATCH 1/1] drm/bridge: analogix_dp: add a quirk for Bob panel @ 2023-02-08 3:15 ` Kencp huang 0 siblings, 0 replies; 6+ messages in thread From: Kencp huang @ 2023-02-08 3:15 UTC (permalink / raw) To: a.hajda, armstrong, robert.foss, Laurent.pinchart, jonas, jernej.skrabec, airlied, daniel Cc: dri-devel, linux-kernel, kencp_huang, briannorris, seanpaul, hoegsberg, wzz, zyw, Kencp huang From: zain wang <wzz@rock-chips.com> 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 <wzz@rock-chips.com> Signed-off-by: Chris Zhong <zyw@rock-chips.com> Commit-Ready: Kristian H. Kristensen <hoegsberg@chromium.org> Tested-by: Kristian H. Kristensen <hoegsberg@chromium.org> Reviewed-by: Kristian H. Kristensen <hoegsberg@chromium.org> Tested-by: Kencp huang <kencp_huang@asus.corp-partner.google.com> Signed-off-by: Kencp huang <kencp_huang@asus.corp-partner.google.com> --- .../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, ®); + 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 1/1] drm/bridge: analogix_dp: add a quirk for Bob panel @ 2023-02-08 3:15 ` Kencp huang 0 siblings, 0 replies; 6+ messages in thread From: Kencp huang @ 2023-02-08 3:15 UTC (permalink / raw) To: a.hajda, armstrong, robert.foss, Laurent.pinchart, jonas, jernej.skrabec, airlied, daniel Cc: wzz, briannorris, linux-kernel, dri-devel, Kencp huang, kencp_huang, seanpaul, zyw, hoegsberg From: zain wang <wzz@rock-chips.com> 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 <wzz@rock-chips.com> Signed-off-by: Chris Zhong <zyw@rock-chips.com> Commit-Ready: Kristian H. Kristensen <hoegsberg@chromium.org> Tested-by: Kristian H. Kristensen <hoegsberg@chromium.org> Reviewed-by: Kristian H. Kristensen <hoegsberg@chromium.org> Tested-by: Kencp huang <kencp_huang@asus.corp-partner.google.com> Signed-off-by: Kencp huang <kencp_huang@asus.corp-partner.google.com> --- .../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, ®); + 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 1/1] drm/bridge: analogix_dp: add a quirk for Bob panel 2023-02-08 3:15 ` Kencp huang @ 2023-02-08 4:44 ` Kencp huang -1 siblings, 0 replies; 6+ messages in thread From: Kencp huang @ 2023-02-08 4:44 UTC (permalink / raw) To: kencp_huang Cc: Laurent.pinchart, andrzej.hajda, airlied, neil.armstrong, briannorris, daniel, dri-devel, hoegsberg, jernej.skrabec, jonas, kencp_huang, linux-kernel, rfoss, seanpaul, wzz, zyw From: zain wang <wzz@rock-chips.com> 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 <wzz@rock-chips.com> Signed-off-by: Chris Zhong <zyw@rock-chips.com> Commit-Ready: Kristian H. Kristensen <hoegsberg@chromium.org> Tested-by: Kristian H. Kristensen <hoegsberg@chromium.org> Reviewed-by: Kristian H. Kristensen <hoegsberg@chromium.org> Tested-by: Kencp huang <kencp_huang@asus.corp-partner.google.com> Signed-off-by: Kencp huang <kencp_huang@asus.corp-partner.google.com> --- .../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, ®); + 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 1/1] drm/bridge: analogix_dp: add a quirk for Bob panel @ 2023-02-08 4:44 ` Kencp huang 0 siblings, 0 replies; 6+ messages in thread From: Kencp huang @ 2023-02-08 4:44 UTC (permalink / raw) To: kencp_huang Cc: neil.armstrong, jernej.skrabec, rfoss, andrzej.hajda, jonas, briannorris, linux-kernel, dri-devel, kencp_huang, wzz, seanpaul, Laurent.pinchart, zyw, hoegsberg From: zain wang <wzz@rock-chips.com> 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 <wzz@rock-chips.com> Signed-off-by: Chris Zhong <zyw@rock-chips.com> Commit-Ready: Kristian H. Kristensen <hoegsberg@chromium.org> Tested-by: Kristian H. Kristensen <hoegsberg@chromium.org> Reviewed-by: Kristian H. Kristensen <hoegsberg@chromium.org> Tested-by: Kencp huang <kencp_huang@asus.corp-partner.google.com> Signed-off-by: Kencp huang <kencp_huang@asus.corp-partner.google.com> --- .../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, ®); + 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] drm/bridge: analogix_dp: add a quirk for Bob panel 2023-02-08 4:44 ` Kencp huang @ 2023-02-14 21:02 ` Brian Norris -1 siblings, 0 replies; 6+ messages in thread From: Brian Norris @ 2023-02-14 21:02 UTC (permalink / raw) To: Kencp huang Cc: Laurent.pinchart, andrzej.hajda, airlied, neil.armstrong, daniel, dri-devel, hoegsberg, jernej.skrabec, jonas, kencp_huang, linux-kernel, rfoss, seanpaul, wzz, zyw 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 <wzz@rock-chips.com> > > 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 <wzz@rock-chips.com> > Signed-off-by: Chris Zhong <zyw@rock-chips.com> > Commit-Ready: Kristian H. Kristensen <hoegsberg@chromium.org> '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 <hoegsberg@chromium.org> > Reviewed-by: Kristian H. Kristensen <hoegsberg@chromium.org> > Tested-by: Kencp huang <kencp_huang@asus.corp-partner.google.com> > Signed-off-by: Kencp huang <kencp_huang@asus.corp-partner.google.com> > --- > .../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, ®); > + 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 <briannorris@chromium.org> > + 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 > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] drm/bridge: analogix_dp: add a quirk for Bob panel @ 2023-02-14 21:02 ` Brian Norris 0 siblings, 0 replies; 6+ messages in thread From: Brian Norris @ 2023-02-14 21:02 UTC (permalink / raw) To: Kencp huang Cc: neil.armstrong, jernej.skrabec, rfoss, andrzej.hajda, jonas, linux-kernel, dri-devel, kencp_huang, wzz, seanpaul, Laurent.pinchart, zyw, hoegsberg 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 <wzz@rock-chips.com> > > 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 <wzz@rock-chips.com> > Signed-off-by: Chris Zhong <zyw@rock-chips.com> > Commit-Ready: Kristian H. Kristensen <hoegsberg@chromium.org> '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 <hoegsberg@chromium.org> > Reviewed-by: Kristian H. Kristensen <hoegsberg@chromium.org> > Tested-by: Kencp huang <kencp_huang@asus.corp-partner.google.com> > Signed-off-by: Kencp huang <kencp_huang@asus.corp-partner.google.com> > --- > .../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, ®); > + 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 <briannorris@chromium.org> > + 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 > ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-02-14 21:02 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-02-08 3:15 [PATCH 1/1] drm/bridge: analogix_dp: add a quirk for Bob panel Kencp huang 2023-02-08 3:15 ` Kencp huang 2023-02-08 4:44 ` Kencp huang 2023-02-08 4:44 ` Kencp huang 2023-02-14 21:02 ` Brian Norris 2023-02-14 21:02 ` Brian Norris
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.