Date:   Tue, 14 Feb 2023 13:02:15 -0800
From:   Brian Norris <briannorris@chromium.org>
To:     Kencp huang <kencp_huang@asus.corp-partner.google.com>
Cc:     Laurent.pinchart@ideasonboard.com, andrzej.hajda@intel.com,
        airlied@gmail.com, neil.armstrong@linaro.org, daniel@ffwll.ch,
        dri-devel@lists.freedesktop.org, hoegsberg@chromium.org,
        jernej.skrabec@gmail.com, jonas@kwiboo.se, kencp_huang@asus.com,
        linux-kernel@vger.kernel.org, rfoss@kernel.org,
        seanpaul@chromium.org, wzz@rock-chips.com, zyw@rock-chips.com
Subject: Re: [PATCH 1/1] drm/bridge: analogix_dp: add a quirk for Bob panel
Message-ID: <Y+v216m4Ba+tiIlt@google.com>
References: <20230208031519.7440-1-kencp_huang@asus.corp-partner.google.com>
 <20230208044406.8280-1-kencp_huang@asus.corp-partner.google.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230208044406.8280-1-kencp_huang@asus.corp-partner.google.com>
Precedence: bulk

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, &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 <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
>