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@xxxxxxxxxxxxxx>
>
> 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@xxxxxxxxxxxxxx>
> Signed-off-by: Chris Zhong <zyw@xxxxxxxxxxxxxx>
> Commit-Ready: Kristian H. Kristensen <hoegsberg@xxxxxxxxxxxx>
'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@xxxxxxxxxxxx>
> Reviewed-by: Kristian H. Kristensen <hoegsberg@xxxxxxxxxxxx>
> Tested-by: Kencp huang <kencp_huang@xxxxxxxxxxxxxxxxxxxxxxxxxxxx>
> Signed-off-by: Kencp huang <kencp_huang@xxxxxxxxxxxxxxxxxxxxxxxxxxxx>
> ---
> .../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@xxxxxxxxxxxx>
> + 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
>
