diff mbox series

[v3,3/5] drm/i915/panelreplay: Initializaton and compute config for panel replay

Message ID 20211010121039.14725-4-animesh.manna@intel.com (mailing list archive)
State New, archived
Headers show
Series Panel replay phase1 implementation | expand

Commit Message

Manna, Animesh Oct. 10, 2021, 12:10 p.m. UTC
As panel replay feature similar to PSR feature of EDP panel, so currently
utilized existing psr framework for panel replay.

v1: RFC version.
v2: optimized code, pr_enabled and pr_dpcd variable removed. [Jose]
v3:
- code comments improved. [Jani]
- dpcd_readb used instead of dpcd_read. [Jani]
- panel-repaplay init/compute functions moved inside respective psr
function. [Jani]

Signed-off-by: Animesh Manna <animesh.manna@intel.com>
---
 .../drm/i915/display/intel_display_types.h    |  2 +
 drivers/gpu/drm/i915/display/intel_dp.c       | 43 +++++++++++++----
 drivers/gpu/drm/i915/display/intel_psr.c      | 48 +++++++++++++++++++
 drivers/gpu/drm/i915/display/intel_psr.h      |  3 ++
 4 files changed, 87 insertions(+), 9 deletions(-)

Comments

kernel test robot Oct. 10, 2021, 6:21 p.m. UTC | #1
Hi Animesh,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on drm-tip/drm-tip]
[also build test WARNING on next-20211008]
[cannot apply to drm-intel/for-linux-next drm-exynos/exynos-drm-next tegra-drm/drm/tegra/for-next linus/master airlied/drm-next v5.15-rc4]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Animesh-Manna/Panel-replay-phase1-implementation/20211010-203447
base:   git://anongit.freedesktop.org/drm/drm-tip drm-tip
config: i386-randconfig-a014-20211010 (attached as .config)
compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project 903b30fea21f99d8f48fde4defcc838970e30ee1)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/474a8190321f2836a1fe989326736d19dc9a732b
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Animesh-Manna/Panel-replay-phase1-implementation/20211010-203447
        git checkout 474a8190321f2836a1fe989326736d19dc9a732b
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 ARCH=i386 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> drivers/gpu/drm/i915/display/intel_dp.c:2780:27: warning: overlapping comparisons always evaluate to true [-Wtautological-overlap-compare]
           if (vsc->revision != 0x5 || vsc->revision != 0x7)
               ~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~
   1 warning generated.


vim +2780 drivers/gpu/drm/i915/display/intel_dp.c

  2756	
  2757	static ssize_t intel_dp_vsc_sdp_pack(const struct drm_dp_vsc_sdp *vsc,
  2758					     struct dp_sdp *sdp, size_t size)
  2759	{
  2760		size_t length = sizeof(struct dp_sdp);
  2761	
  2762		if (size < length)
  2763			return -ENOSPC;
  2764	
  2765		memset(sdp, 0, size);
  2766	
  2767		/*
  2768		 * Prepare VSC Header for SU as per DP 1.4a spec, Table 2-119
  2769		 * VSC SDP Header Bytes
  2770		 */
  2771		sdp->sdp_header.HB0 = 0; /* Secondary-Data Packet ID = 0 */
  2772		sdp->sdp_header.HB1 = vsc->sdp_type; /* Secondary-data Packet Type */
  2773		sdp->sdp_header.HB2 = vsc->revision; /* Revision Number */
  2774		sdp->sdp_header.HB3 = vsc->length; /* Number of Valid Data Bytes */
  2775	
  2776		/*
  2777		 * Revision 0x5 and 0x7 supports Pixel Encoding/Colorimetry Format as
  2778		 * per DP 1.4a spec and DP 2.0 spec respectively.
  2779		 */
> 2780		if (vsc->revision != 0x5 || vsc->revision != 0x7)
  2781			goto out;
  2782	
  2783		/* VSC SDP Payload for DB16 through DB18 */
  2784		/* Pixel Encoding and Colorimetry Formats  */
  2785		sdp->db[16] = (vsc->pixelformat & 0xf) << 4; /* DB16[7:4] */
  2786		sdp->db[16] |= vsc->colorimetry & 0xf; /* DB16[3:0] */
  2787	
  2788		switch (vsc->bpc) {
  2789		case 6:
  2790			/* 6bpc: 0x0 */
  2791			break;
  2792		case 8:
  2793			sdp->db[17] = 0x1; /* DB17[3:0] */
  2794			break;
  2795		case 10:
  2796			sdp->db[17] = 0x2;
  2797			break;
  2798		case 12:
  2799			sdp->db[17] = 0x3;
  2800			break;
  2801		case 16:
  2802			sdp->db[17] = 0x4;
  2803			break;
  2804		default:
  2805			MISSING_CASE(vsc->bpc);
  2806			break;
  2807		}
  2808		/* Dynamic Range and Component Bit Depth */
  2809		if (vsc->dynamic_range == DP_DYNAMIC_RANGE_CTA)
  2810			sdp->db[17] |= 0x80;  /* DB17[7] */
  2811	
  2812		/* Content Type */
  2813		sdp->db[18] = vsc->content_type & 0x7;
  2814	
  2815	out:
  2816		return length;
  2817	}
  2818	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
kernel test robot Oct. 10, 2021, 8:28 p.m. UTC | #2
Hi Animesh,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on drm-tip/drm-tip]
[also build test ERROR on next-20211008]
[cannot apply to drm-intel/for-linux-next drm-exynos/exynos-drm-next tegra-drm/drm/tegra/for-next linus/master airlied/drm-next v5.15-rc4]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Animesh-Manna/Panel-replay-phase1-implementation/20211010-203447
base:   git://anongit.freedesktop.org/drm/drm-tip drm-tip
config: i386-randconfig-r014-20211010 (attached as .config)
compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project 903b30fea21f99d8f48fde4defcc838970e30ee1)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/474a8190321f2836a1fe989326736d19dc9a732b
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Animesh-Manna/Panel-replay-phase1-implementation/20211010-203447
        git checkout 474a8190321f2836a1fe989326736d19dc9a732b
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 ARCH=i386 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> drivers/gpu/drm/i915/display/intel_dp.c:2780:27: error: overlapping comparisons always evaluate to true [-Werror,-Wtautological-overlap-compare]
           if (vsc->revision != 0x5 || vsc->revision != 0x7)
               ~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~
   1 error generated.


vim +2780 drivers/gpu/drm/i915/display/intel_dp.c

  2756	
  2757	static ssize_t intel_dp_vsc_sdp_pack(const struct drm_dp_vsc_sdp *vsc,
  2758					     struct dp_sdp *sdp, size_t size)
  2759	{
  2760		size_t length = sizeof(struct dp_sdp);
  2761	
  2762		if (size < length)
  2763			return -ENOSPC;
  2764	
  2765		memset(sdp, 0, size);
  2766	
  2767		/*
  2768		 * Prepare VSC Header for SU as per DP 1.4a spec, Table 2-119
  2769		 * VSC SDP Header Bytes
  2770		 */
  2771		sdp->sdp_header.HB0 = 0; /* Secondary-Data Packet ID = 0 */
  2772		sdp->sdp_header.HB1 = vsc->sdp_type; /* Secondary-data Packet Type */
  2773		sdp->sdp_header.HB2 = vsc->revision; /* Revision Number */
  2774		sdp->sdp_header.HB3 = vsc->length; /* Number of Valid Data Bytes */
  2775	
  2776		/*
  2777		 * Revision 0x5 and 0x7 supports Pixel Encoding/Colorimetry Format as
  2778		 * per DP 1.4a spec and DP 2.0 spec respectively.
  2779		 */
> 2780		if (vsc->revision != 0x5 || vsc->revision != 0x7)
  2781			goto out;
  2782	
  2783		/* VSC SDP Payload for DB16 through DB18 */
  2784		/* Pixel Encoding and Colorimetry Formats  */
  2785		sdp->db[16] = (vsc->pixelformat & 0xf) << 4; /* DB16[7:4] */
  2786		sdp->db[16] |= vsc->colorimetry & 0xf; /* DB16[3:0] */
  2787	
  2788		switch (vsc->bpc) {
  2789		case 6:
  2790			/* 6bpc: 0x0 */
  2791			break;
  2792		case 8:
  2793			sdp->db[17] = 0x1; /* DB17[3:0] */
  2794			break;
  2795		case 10:
  2796			sdp->db[17] = 0x2;
  2797			break;
  2798		case 12:
  2799			sdp->db[17] = 0x3;
  2800			break;
  2801		case 16:
  2802			sdp->db[17] = 0x4;
  2803			break;
  2804		default:
  2805			MISSING_CASE(vsc->bpc);
  2806			break;
  2807		}
  2808		/* Dynamic Range and Component Bit Depth */
  2809		if (vsc->dynamic_range == DP_DYNAMIC_RANGE_CTA)
  2810			sdp->db[17] |= 0x80;  /* DB17[7] */
  2811	
  2812		/* Content Type */
  2813		sdp->db[18] = vsc->content_type & 0x7;
  2814	
  2815	out:
  2816		return length;
  2817	}
  2818	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Souza, Jose Nov. 23, 2021, 7:48 p.m. UTC | #3
On Sun, 2021-10-10 at 17:40 +0530, Animesh Manna wrote:
> As panel replay feature similar to PSR feature of EDP panel, so currently
> utilized existing psr framework for panel replay.
> 
> v1: RFC version.
> v2: optimized code, pr_enabled and pr_dpcd variable removed. [Jose]
> v3:
> - code comments improved. [Jani]
> - dpcd_readb used instead of dpcd_read. [Jani]
> - panel-repaplay init/compute functions moved inside respective psr
> function. [Jani]
> 
> Signed-off-by: Animesh Manna <animesh.manna@intel.com>
> ---
>  .../drm/i915/display/intel_display_types.h    |  2 +
>  drivers/gpu/drm/i915/display/intel_dp.c       | 43 +++++++++++++----
>  drivers/gpu/drm/i915/display/intel_psr.c      | 48 +++++++++++++++++++
>  drivers/gpu/drm/i915/display/intel_psr.h      |  3 ++
>  4 files changed, 87 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
> index 39e11eaec1a3..48f7d676ed2c 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> @@ -1070,6 +1070,7 @@ struct intel_crtc_state {
>  	bool req_psr2_sdp_prior_scanline;
>  	u32 dc3co_exitline;
>  	u16 su_y_granularity;
> +	bool has_panel_replay;

We can drop this and reuse current ones ones, see bellow.

>  	struct drm_dp_vsc_sdp psr_vsc;
>  
>  	/*
> @@ -1531,6 +1532,7 @@ struct intel_psr {
>  	bool irq_aux_error;
>  	u16 su_w_granularity;
>  	u16 su_y_granularity;
> +	bool sink_panel_replay_support;

move this closer to has_psr and set both when it is panel replay.
otherwise psr functions will not be executed for panel replay, see CAN_PSR().

>  	u32 dc3co_exitline;
>  	u32 dc3co_exit_delay;
>  	struct delayed_work dc3co_work;
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> index 10fda20a5bd8..f58a7b72be14 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -1587,12 +1587,22 @@ static void intel_dp_compute_vsc_colorimetry(const struct intel_crtc_state *crtc
>  	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
>  	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>  
> -	/*
> -	 * Prepare VSC Header for SU as per DP 1.4 spec, Table 2-118
> -	 * VSC SDP supporting 3D stereo, PSR2, and Pixel Encoding/
> -	 * Colorimetry Format indication.
> -	 */
> -	vsc->revision = 0x5;
> +	if (crtc_state->has_panel_replay) {
> +		/*
> +		 * Prepare VSC Header for SU as per DP 2.0 spec, Table 2-223
> +		 * VSC SDP supporting 3D stereo, Panel Replay, and Pixel
> +		 * Encoding/Colorimetry Format indication.
> +		 */
> +		vsc->revision = 0x7;
> +	} else {
> +		/*
> +		 * Prepare VSC Header for SU as per DP 1.4 spec, Table 2-118
> +		 * VSC SDP supporting 3D stereo, PSR2, and Pixel Encoding/
> +		 * Colorimetry Format indication.
> +		 */
> +		vsc->revision = 0x5;
> +	}
> +
>  	vsc->length = 0x13;
>  
>  	/* DP 1.4a spec, Table 2-120 */
> @@ -1701,6 +1711,21 @@ void intel_dp_compute_psr_vsc_sdp(struct intel_dp *intel_dp,
>  			vsc->revision = 0x4;
>  			vsc->length = 0xe;
>  		}
> +	} else if (intel_dp->psr.enabled && !intel_dp_is_edp(intel_dp)) {
> +		if (intel_dp->psr.colorimetry_support &&
> +		    intel_dp_needs_vsc_sdp(crtc_state, conn_state)) {
> +			/* [Panel Replay with colorimetry info] */
> +			intel_dp_compute_vsc_colorimetry(crtc_state, conn_state,
> +							 vsc);
> +		} else {
> +			/*
> +			 * [Panel Replay without colorimetry info]
> +			 * Prepare VSC Header for SU as per DP 2.0 spec, Table 2-223
> +			 * VSC SDP supporting 3D stereo + Panel Replay.
> +			 */
> +			vsc->revision = 0x6;
> +			vsc->length = 0x10;
> +		}
>  	} else {
>  		/*
>  		 * [PSR1]
> @@ -2749,10 +2774,10 @@ static ssize_t intel_dp_vsc_sdp_pack(const struct drm_dp_vsc_sdp *vsc,
>  	sdp->sdp_header.HB3 = vsc->length; /* Number of Valid Data Bytes */
>  
>  	/*
> -	 * Only revision 0x5 supports Pixel Encoding/Colorimetry Format as
> -	 * per DP 1.4a spec.
> +	 * Revision 0x5 and 0x7 supports Pixel Encoding/Colorimetry Format as
> +	 * per DP 1.4a spec and DP 2.0 spec respectively.
>  	 */
> -	if (vsc->revision != 0x5)
> +	if (vsc->revision != 0x5 || vsc->revision != 0x7)
>  		goto out;
>  
>  	/* VSC SDP Payload for DB16 through DB18 */
> diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c
> index 7a205fd5023b..91c2efe2f3ad 100644
> --- a/drivers/gpu/drm/i915/display/intel_psr.c
> +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> @@ -933,6 +933,21 @@ static bool intel_psr2_config_valid(struct intel_dp *intel_dp,
>  	return true;
>  }
>  
> +void intel_panel_replay_compute_config(struct intel_dp *intel_dp,
> +				       struct intel_crtc_state *crtc_state)
> +{
> +	struct drm_i915_private *i915 = dp_to_i915(intel_dp);
> +
> +	if (!intel_dp->psr.sink_panel_replay_support)
> +		return;
> +
> +	crtc_state->has_panel_replay = true;
> +	crtc_state->infoframes.enable |= intel_hdmi_infoframe_enable(DP_SDP_VSC);
> +
> +	if (HAS_PSR2_SEL_FETCH(i915))
> +		intel_psr2_sel_fetch_config_valid(intel_dp, crtc_state);
> +}
> +
>  void intel_psr_compute_config(struct intel_dp *intel_dp,
>  			      struct intel_crtc_state *crtc_state,
>  			      struct drm_connector_state *conn_state)
> @@ -942,6 +957,8 @@ void intel_psr_compute_config(struct intel_dp *intel_dp,
>  		&crtc_state->hw.adjusted_mode;
>  	int psr_setup_time;
>  
> +	intel_panel_replay_compute_config(intel_dp, crtc_state);

have you checked if the other PSR are needed for panel replay? what about the psr2 checks? when using panel replay selective update some additional
tests will be needed regarding granularity...

> +
>  	/*
>  	 * Current PSR panels dont work reliably with VRR enabled
>  	 * So if VRR is enabled, do not enable PSR.
> @@ -2170,6 +2187,35 @@ void intel_psr_flush(struct drm_i915_private *dev_priv,
>  	}
>  }
>  
> +/**
> + * intel_panel_replay_init - Check for sink and source capability.
> + * @intel_dp: Intel DP
> + *
> + * This function is called after the initializing connector.
> + * (the initializing of connector treats the handling of connector capabilities)
> + * And it initializes basic panel replay stuff for each DP Encoder.
> + */
> +void intel_panel_replay_init(struct intel_dp *intel_dp)
> +{
> +	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> +	u8 pr_dpcd = 0;
> +
> +	if (!(HAS_DP20(dev_priv) && HAS_PANEL_REPLAY(dev_priv)))
> +		return;
> +
> +	drm_dp_dpcd_readb(&intel_dp->aux, DP_PANEL_REPLAY_CAP, &pr_dpcd);
> +
> +	if (!(pr_dpcd & PANEL_REPLAY_SUPPORT)) {
> +		drm_dbg_kms(&dev_priv->drm,
> +			    "Panel replay is not supported by panel\n");
> +		return;
> +	}
> +
> +	drm_dbg_kms(&dev_priv->drm,
> +		    "Panel replay is supported by panel\n");
> +	intel_dp->psr.sink_panel_replay_support = true;
> +}
> +
>  /**
>   * intel_psr_init - Init basic PSR work and mutex.
>   * @intel_dp: Intel DP
> @@ -2183,6 +2229,8 @@ void intel_psr_init(struct intel_dp *intel_dp)
>  	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
>  	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
>  
> +	intel_panel_replay_init(intel_dp);
> +
>  	if (!HAS_PSR(dev_priv))
>  		return;

mutex initialization is not executed, workers not initialized...
please go more carefully trough every PSR function and check what are the panel replay implications

>  
> diff --git a/drivers/gpu/drm/i915/display/intel_psr.h b/drivers/gpu/drm/i915/display/intel_psr.h
> index facffbacd357..c9d1c1f0b470 100644
> --- a/drivers/gpu/drm/i915/display/intel_psr.h
> +++ b/drivers/gpu/drm/i915/display/intel_psr.h
> @@ -32,6 +32,7 @@ void intel_psr_flush(struct drm_i915_private *dev_priv,
>  		     unsigned frontbuffer_bits,
>  		     enum fb_op_origin origin);
>  void intel_psr_init(struct intel_dp *intel_dp);
> +void intel_panel_replay_init(struct intel_dp *intel_dp);
>  void intel_psr_compute_config(struct intel_dp *intel_dp,
>  			      struct intel_crtc_state *crtc_state,
>  			      struct drm_connector_state *conn_state);
> @@ -52,5 +53,7 @@ void intel_psr2_disable_plane_sel_fetch(struct intel_plane *plane,
>  					const struct intel_crtc_state *crtc_state);
>  void intel_psr_pause(struct intel_dp *intel_dp);
>  void intel_psr_resume(struct intel_dp *intel_dp);
> +void intel_panel_replay_compute_config(struct intel_dp *intel_dp,
> +				       struct intel_crtc_state *crtc_state);
>  
>  #endif /* __INTEL_PSR_H__ */
Manna, Animesh Jan. 4, 2022, 3:51 p.m. UTC | #4
Hi,

> -----Original Message-----
> From: Souza, Jose <jose.souza@intel.com>
> Sent: Wednesday, November 24, 2021 1:19 AM
> To: dri-devel@lists.freedesktop.org; Manna, Animesh
> <animesh.manna@intel.com>; intel-gfx@lists.freedesktop.org
> Cc: Mun, Gwan-gyeong <gwan-gyeong.mun@intel.com>; Nikula, Jani
> <jani.nikula@intel.com>; Kahola, Mika <mika.kahola@intel.com>; Navare,
> Manasi D <manasi.d.navare@intel.com>
> Subject: Re: [PATCH v3 3/5] drm/i915/panelreplay: Initializaton and compute
> config for panel replay
> 
> On Sun, 2021-10-10 at 17:40 +0530, Animesh Manna wrote:
> > As panel replay feature similar to PSR feature of EDP panel, so
> > currently utilized existing psr framework for panel replay.
> >
> > v1: RFC version.
> > v2: optimized code, pr_enabled and pr_dpcd variable removed. [Jose]
> > v3:
> > - code comments improved. [Jani]
> > - dpcd_readb used instead of dpcd_read. [Jani]
> > - panel-repaplay init/compute functions moved inside respective psr
> > function. [Jani]
> >
> > Signed-off-by: Animesh Manna <animesh.manna@intel.com>
> > ---
> >  .../drm/i915/display/intel_display_types.h    |  2 +
> >  drivers/gpu/drm/i915/display/intel_dp.c       | 43 +++++++++++++----
> >  drivers/gpu/drm/i915/display/intel_psr.c      | 48 +++++++++++++++++++
> >  drivers/gpu/drm/i915/display/intel_psr.h      |  3 ++
> >  4 files changed, 87 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h
> > b/drivers/gpu/drm/i915/display/intel_display_types.h
> > index 39e11eaec1a3..48f7d676ed2c 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> > @@ -1070,6 +1070,7 @@ struct intel_crtc_state {
> >  	bool req_psr2_sdp_prior_scanline;
> >  	u32 dc3co_exitline;
> >  	u16 su_y_granularity;
> > +	bool has_panel_replay;
> 
> We can drop this and reuse current ones ones, see bellow.
> 
> >  	struct drm_dp_vsc_sdp psr_vsc;
> >
> >  	/*
> > @@ -1531,6 +1532,7 @@ struct intel_psr {
> >  	bool irq_aux_error;
> >  	u16 su_w_granularity;
> >  	u16 su_y_granularity;
> > +	bool sink_panel_replay_support;
> 
> move this closer to has_psr and set both when it is panel replay.
> otherwise psr functions will not be executed for panel replay, see CAN_PSR().

AFIU Panel replay do not have any dependency with PSR.
So that’s why I have created separate function for panel replay which is doing similar thing whatever needed for panel replay.
For example intel_panel_replay_compute_config() can be independent of intel_psr_compute_config().
Do you see any dependency with PSR for panel replay?

> 
> >  	u32 dc3co_exitline;
> >  	u32 dc3co_exit_delay;
> >  	struct delayed_work dc3co_work;
> > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
> > b/drivers/gpu/drm/i915/display/intel_dp.c
> > index 10fda20a5bd8..f58a7b72be14 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > @@ -1587,12 +1587,22 @@ static void
> intel_dp_compute_vsc_colorimetry(const struct intel_crtc_state *crtc
> >  	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> >  	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> >
> > -	/*
> > -	 * Prepare VSC Header for SU as per DP 1.4 spec, Table 2-118
> > -	 * VSC SDP supporting 3D stereo, PSR2, and Pixel Encoding/
> > -	 * Colorimetry Format indication.
> > -	 */
> > -	vsc->revision = 0x5;
> > +	if (crtc_state->has_panel_replay) {
> > +		/*
> > +		 * Prepare VSC Header for SU as per DP 2.0 spec, Table 2-223
> > +		 * VSC SDP supporting 3D stereo, Panel Replay, and Pixel
> > +		 * Encoding/Colorimetry Format indication.
> > +		 */
> > +		vsc->revision = 0x7;
> > +	} else {
> > +		/*
> > +		 * Prepare VSC Header for SU as per DP 1.4 spec, Table 2-118
> > +		 * VSC SDP supporting 3D stereo, PSR2, and Pixel Encoding/
> > +		 * Colorimetry Format indication.
> > +		 */
> > +		vsc->revision = 0x5;
> > +	}
> > +
> >  	vsc->length = 0x13;
> >
> >  	/* DP 1.4a spec, Table 2-120 */
> > @@ -1701,6 +1711,21 @@ void intel_dp_compute_psr_vsc_sdp(struct
> intel_dp *intel_dp,
> >  			vsc->revision = 0x4;
> >  			vsc->length = 0xe;
> >  		}
> > +	} else if (intel_dp->psr.enabled && !intel_dp_is_edp(intel_dp)) {
> > +		if (intel_dp->psr.colorimetry_support &&
> > +		    intel_dp_needs_vsc_sdp(crtc_state, conn_state)) {
> > +			/* [Panel Replay with colorimetry info] */
> > +			intel_dp_compute_vsc_colorimetry(crtc_state,
> conn_state,
> > +							 vsc);
> > +		} else {
> > +			/*
> > +			 * [Panel Replay without colorimetry info]
> > +			 * Prepare VSC Header for SU as per DP 2.0 spec, Table
> 2-223
> > +			 * VSC SDP supporting 3D stereo + Panel Replay.
> > +			 */
> > +			vsc->revision = 0x6;
> > +			vsc->length = 0x10;
> > +		}
> >  	} else {
> >  		/*
> >  		 * [PSR1]
> > @@ -2749,10 +2774,10 @@ static ssize_t intel_dp_vsc_sdp_pack(const struct
> drm_dp_vsc_sdp *vsc,
> >  	sdp->sdp_header.HB3 = vsc->length; /* Number of Valid Data Bytes */
> >
> >  	/*
> > -	 * Only revision 0x5 supports Pixel Encoding/Colorimetry Format as
> > -	 * per DP 1.4a spec.
> > +	 * Revision 0x5 and 0x7 supports Pixel Encoding/Colorimetry Format as
> > +	 * per DP 1.4a spec and DP 2.0 spec respectively.
> >  	 */
> > -	if (vsc->revision != 0x5)
> > +	if (vsc->revision != 0x5 || vsc->revision != 0x7)
> >  		goto out;
> >
> >  	/* VSC SDP Payload for DB16 through DB18 */ diff --git
> > a/drivers/gpu/drm/i915/display/intel_psr.c
> > b/drivers/gpu/drm/i915/display/intel_psr.c
> > index 7a205fd5023b..91c2efe2f3ad 100644
> > --- a/drivers/gpu/drm/i915/display/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> > @@ -933,6 +933,21 @@ static bool intel_psr2_config_valid(struct intel_dp
> *intel_dp,
> >  	return true;
> >  }
> >
> > +void intel_panel_replay_compute_config(struct intel_dp *intel_dp,
> > +				       struct intel_crtc_state *crtc_state) {
> > +	struct drm_i915_private *i915 = dp_to_i915(intel_dp);
> > +
> > +	if (!intel_dp->psr.sink_panel_replay_support)
> > +		return;
> > +
> > +	crtc_state->has_panel_replay = true;
> > +	crtc_state->infoframes.enable |=
> > +intel_hdmi_infoframe_enable(DP_SDP_VSC);
> > +
> > +	if (HAS_PSR2_SEL_FETCH(i915))
> > +		intel_psr2_sel_fetch_config_valid(intel_dp, crtc_state); }
> > +
> >  void intel_psr_compute_config(struct intel_dp *intel_dp,
> >  			      struct intel_crtc_state *crtc_state,
> >  			      struct drm_connector_state *conn_state) @@ -
> 942,6 +957,8 @@
> > void intel_psr_compute_config(struct intel_dp *intel_dp,
> >  		&crtc_state->hw.adjusted_mode;
> >  	int psr_setup_time;
> >
> > +	intel_panel_replay_compute_config(intel_dp, crtc_state);
> 
> have you checked if the other PSR are needed for panel replay? what about the
> psr2 checks? when using panel replay selective update some additional tests will
> be needed regarding granularity...

As mentioned above I understood panel replay do not have any dependency with PSR.
Will not target panel replay selective update for dg2.

> 
> > +
> >  	/*
> >  	 * Current PSR panels dont work reliably with VRR enabled
> >  	 * So if VRR is enabled, do not enable PSR.
> > @@ -2170,6 +2187,35 @@ void intel_psr_flush(struct drm_i915_private
> *dev_priv,
> >  	}
> >  }
> >
> > +/**
> > + * intel_panel_replay_init - Check for sink and source capability.
> > + * @intel_dp: Intel DP
> > + *
> > + * This function is called after the initializing connector.
> > + * (the initializing of connector treats the handling of connector
> > +capabilities)
> > + * And it initializes basic panel replay stuff for each DP Encoder.
> > + */
> > +void intel_panel_replay_init(struct intel_dp *intel_dp) {
> > +	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> > +	u8 pr_dpcd = 0;
> > +
> > +	if (!(HAS_DP20(dev_priv) && HAS_PANEL_REPLAY(dev_priv)))
> > +		return;
> > +
> > +	drm_dp_dpcd_readb(&intel_dp->aux, DP_PANEL_REPLAY_CAP,
> &pr_dpcd);
> > +
> > +	if (!(pr_dpcd & PANEL_REPLAY_SUPPORT)) {
> > +		drm_dbg_kms(&dev_priv->drm,
> > +			    "Panel replay is not supported by panel\n");
> > +		return;
> > +	}
> > +
> > +	drm_dbg_kms(&dev_priv->drm,
> > +		    "Panel replay is supported by panel\n");
> > +	intel_dp->psr.sink_panel_replay_support = true; }
> > +
> >  /**
> >   * intel_psr_init - Init basic PSR work and mutex.
> >   * @intel_dp: Intel DP
> > @@ -2183,6 +2229,8 @@ void intel_psr_init(struct intel_dp *intel_dp)
> >  	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
> >  	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> >
> > +	intel_panel_replay_init(intel_dp);
> > +
> >  	if (!HAS_PSR(dev_priv))
> >  		return;
> 
> mutex initialization is not executed, workers not initialized...
> please go more carefully trough every PSR function and check what are the
> panel replay implications

Tried to double check once more.
Currently as per bspec and dp 2.0 spec did not see any dependency with PSR.
Not sure if we really need worker thread to enter in panel replay mode unlike psr where we need to wait for few idle frames.
Any suggestion/input will be helpful here.

Regards,
Animesh

> 
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_psr.h
> > b/drivers/gpu/drm/i915/display/intel_psr.h
> > index facffbacd357..c9d1c1f0b470 100644
> > --- a/drivers/gpu/drm/i915/display/intel_psr.h
> > +++ b/drivers/gpu/drm/i915/display/intel_psr.h
> > @@ -32,6 +32,7 @@ void intel_psr_flush(struct drm_i915_private *dev_priv,
> >  		     unsigned frontbuffer_bits,
> >  		     enum fb_op_origin origin);
> >  void intel_psr_init(struct intel_dp *intel_dp);
> > +void intel_panel_replay_init(struct intel_dp *intel_dp);
> >  void intel_psr_compute_config(struct intel_dp *intel_dp,
> >  			      struct intel_crtc_state *crtc_state,
> >  			      struct drm_connector_state *conn_state); @@ -
> 52,5 +53,7 @@
> > void intel_psr2_disable_plane_sel_fetch(struct intel_plane *plane,
> >  					const struct intel_crtc_state
> *crtc_state);  void
> > intel_psr_pause(struct intel_dp *intel_dp);  void
> > intel_psr_resume(struct intel_dp *intel_dp);
> > +void intel_panel_replay_compute_config(struct intel_dp *intel_dp,
> > +				       struct intel_crtc_state *crtc_state);
> >
> >  #endif /* __INTEL_PSR_H__ */
Souza, Jose Jan. 4, 2022, 3:55 p.m. UTC | #5
On Tue, 2022-01-04 at 21:21 +0530, Manna, Animesh wrote:
> Hi,
> 
> > -----Original Message-----
> > From: Souza, Jose <jose.souza@intel.com>
> > Sent: Wednesday, November 24, 2021 1:19 AM
> > To: dri-devel@lists.freedesktop.org; Manna, Animesh
> > <animesh.manna@intel.com>; intel-gfx@lists.freedesktop.org
> > Cc: Mun, Gwan-gyeong <gwan-gyeong.mun@intel.com>; Nikula, Jani
> > <jani.nikula@intel.com>; Kahola, Mika <mika.kahola@intel.com>; Navare,
> > Manasi D <manasi.d.navare@intel.com>
> > Subject: Re: [PATCH v3 3/5] drm/i915/panelreplay: Initializaton and compute
> > config for panel replay
> > 
> > On Sun, 2021-10-10 at 17:40 +0530, Animesh Manna wrote:
> > > As panel replay feature similar to PSR feature of EDP panel, so
> > > currently utilized existing psr framework for panel replay.
> > > 
> > > v1: RFC version.
> > > v2: optimized code, pr_enabled and pr_dpcd variable removed. [Jose]
> > > v3:
> > > - code comments improved. [Jani]
> > > - dpcd_readb used instead of dpcd_read. [Jani]
> > > - panel-repaplay init/compute functions moved inside respective psr
> > > function. [Jani]
> > > 
> > > Signed-off-by: Animesh Manna <animesh.manna@intel.com>
> > > ---
> > >  .../drm/i915/display/intel_display_types.h    |  2 +
> > >  drivers/gpu/drm/i915/display/intel_dp.c       | 43 +++++++++++++----
> > >  drivers/gpu/drm/i915/display/intel_psr.c      | 48 +++++++++++++++++++
> > >  drivers/gpu/drm/i915/display/intel_psr.h      |  3 ++
> > >  4 files changed, 87 insertions(+), 9 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h
> > > b/drivers/gpu/drm/i915/display/intel_display_types.h
> > > index 39e11eaec1a3..48f7d676ed2c 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> > > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> > > @@ -1070,6 +1070,7 @@ struct intel_crtc_state {
> > >  	bool req_psr2_sdp_prior_scanline;
> > >  	u32 dc3co_exitline;
> > >  	u16 su_y_granularity;
> > > +	bool has_panel_replay;
> > 
> > We can drop this and reuse current ones ones, see bellow.
> > 
> > >  	struct drm_dp_vsc_sdp psr_vsc;
> > > 
> > >  	/*
> > > @@ -1531,6 +1532,7 @@ struct intel_psr {
> > >  	bool irq_aux_error;
> > >  	u16 su_w_granularity;
> > >  	u16 su_y_granularity;
> > > +	bool sink_panel_replay_support;
> > 
> > move this closer to has_psr and set both when it is panel replay.
> > otherwise psr functions will not be executed for panel replay, see CAN_PSR().
> 
> AFIU Panel replay do not have any dependency with PSR.
> So that’s why I have created separate function for panel replay which is doing similar thing whatever needed for panel replay.
> For example intel_panel_replay_compute_config() can be independent of intel_psr_compute_config().
> Do you see any dependency with PSR for panel replay?

There is no dependency but panel replay is PSR for DP so we should re-use PSR code as much as possible.

eDP + sink_support = PSR
DP + sink_support = panel replay

So we can reuse has_psr and all other stuff.

> 
> > 
> > >  	u32 dc3co_exitline;
> > >  	u32 dc3co_exit_delay;
> > >  	struct delayed_work dc3co_work;
> > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
> > > b/drivers/gpu/drm/i915/display/intel_dp.c
> > > index 10fda20a5bd8..f58a7b72be14 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > > @@ -1587,12 +1587,22 @@ static void
> > intel_dp_compute_vsc_colorimetry(const struct intel_crtc_state *crtc
> > >  	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> > >  	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> > > 
> > > -	/*
> > > -	 * Prepare VSC Header for SU as per DP 1.4 spec, Table 2-118
> > > -	 * VSC SDP supporting 3D stereo, PSR2, and Pixel Encoding/
> > > -	 * Colorimetry Format indication.
> > > -	 */
> > > -	vsc->revision = 0x5;
> > > +	if (crtc_state->has_panel_replay) {
> > > +		/*
> > > +		 * Prepare VSC Header for SU as per DP 2.0 spec, Table 2-223
> > > +		 * VSC SDP supporting 3D stereo, Panel Replay, and Pixel
> > > +		 * Encoding/Colorimetry Format indication.
> > > +		 */
> > > +		vsc->revision = 0x7;
> > > +	} else {
> > > +		/*
> > > +		 * Prepare VSC Header for SU as per DP 1.4 spec, Table 2-118
> > > +		 * VSC SDP supporting 3D stereo, PSR2, and Pixel Encoding/
> > > +		 * Colorimetry Format indication.
> > > +		 */
> > > +		vsc->revision = 0x5;
> > > +	}
> > > +
> > >  	vsc->length = 0x13;
> > > 
> > >  	/* DP 1.4a spec, Table 2-120 */
> > > @@ -1701,6 +1711,21 @@ void intel_dp_compute_psr_vsc_sdp(struct
> > intel_dp *intel_dp,
> > >  			vsc->revision = 0x4;
> > >  			vsc->length = 0xe;
> > >  		}
> > > +	} else if (intel_dp->psr.enabled && !intel_dp_is_edp(intel_dp)) {
> > > +		if (intel_dp->psr.colorimetry_support &&
> > > +		    intel_dp_needs_vsc_sdp(crtc_state, conn_state)) {
> > > +			/* [Panel Replay with colorimetry info] */
> > > +			intel_dp_compute_vsc_colorimetry(crtc_state,
> > conn_state,
> > > +							 vsc);
> > > +		} else {
> > > +			/*
> > > +			 * [Panel Replay without colorimetry info]
> > > +			 * Prepare VSC Header for SU as per DP 2.0 spec, Table
> > 2-223
> > > +			 * VSC SDP supporting 3D stereo + Panel Replay.
> > > +			 */
> > > +			vsc->revision = 0x6;
> > > +			vsc->length = 0x10;
> > > +		}
> > >  	} else {
> > >  		/*
> > >  		 * [PSR1]
> > > @@ -2749,10 +2774,10 @@ static ssize_t intel_dp_vsc_sdp_pack(const struct
> > drm_dp_vsc_sdp *vsc,
> > >  	sdp->sdp_header.HB3 = vsc->length; /* Number of Valid Data Bytes */
> > > 
> > >  	/*
> > > -	 * Only revision 0x5 supports Pixel Encoding/Colorimetry Format as
> > > -	 * per DP 1.4a spec.
> > > +	 * Revision 0x5 and 0x7 supports Pixel Encoding/Colorimetry Format as
> > > +	 * per DP 1.4a spec and DP 2.0 spec respectively.
> > >  	 */
> > > -	if (vsc->revision != 0x5)
> > > +	if (vsc->revision != 0x5 || vsc->revision != 0x7)
> > >  		goto out;
> > > 
> > >  	/* VSC SDP Payload for DB16 through DB18 */ diff --git
> > > a/drivers/gpu/drm/i915/display/intel_psr.c
> > > b/drivers/gpu/drm/i915/display/intel_psr.c
> > > index 7a205fd5023b..91c2efe2f3ad 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_psr.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> > > @@ -933,6 +933,21 @@ static bool intel_psr2_config_valid(struct intel_dp
> > *intel_dp,
> > >  	return true;
> > >  }
> > > 
> > > +void intel_panel_replay_compute_config(struct intel_dp *intel_dp,
> > > +				       struct intel_crtc_state *crtc_state) {
> > > +	struct drm_i915_private *i915 = dp_to_i915(intel_dp);
> > > +
> > > +	if (!intel_dp->psr.sink_panel_replay_support)
> > > +		return;
> > > +
> > > +	crtc_state->has_panel_replay = true;
> > > +	crtc_state->infoframes.enable |=
> > > +intel_hdmi_infoframe_enable(DP_SDP_VSC);
> > > +
> > > +	if (HAS_PSR2_SEL_FETCH(i915))
> > > +		intel_psr2_sel_fetch_config_valid(intel_dp, crtc_state); }
> > > +
> > >  void intel_psr_compute_config(struct intel_dp *intel_dp,
> > >  			      struct intel_crtc_state *crtc_state,
> > >  			      struct drm_connector_state *conn_state) @@ -
> > 942,6 +957,8 @@
> > > void intel_psr_compute_config(struct intel_dp *intel_dp,
> > >  		&crtc_state->hw.adjusted_mode;
> > >  	int psr_setup_time;
> > > 
> > > +	intel_panel_replay_compute_config(intel_dp, crtc_state);
> > 
> > have you checked if the other PSR are needed for panel replay? what about the
> > psr2 checks? when using panel replay selective update some additional tests will
> > be needed regarding granularity...
> 
> As mentioned above I understood panel replay do not have any dependency with PSR.
> Will not target panel replay selective update for dg2.
> 
> > 
> > > +
> > >  	/*
> > >  	 * Current PSR panels dont work reliably with VRR enabled
> > >  	 * So if VRR is enabled, do not enable PSR.
> > > @@ -2170,6 +2187,35 @@ void intel_psr_flush(struct drm_i915_private
> > *dev_priv,
> > >  	}
> > >  }
> > > 
> > > +/**
> > > + * intel_panel_replay_init - Check for sink and source capability.
> > > + * @intel_dp: Intel DP
> > > + *
> > > + * This function is called after the initializing connector.
> > > + * (the initializing of connector treats the handling of connector
> > > +capabilities)
> > > + * And it initializes basic panel replay stuff for each DP Encoder.
> > > + */
> > > +void intel_panel_replay_init(struct intel_dp *intel_dp) {
> > > +	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> > > +	u8 pr_dpcd = 0;
> > > +
> > > +	if (!(HAS_DP20(dev_priv) && HAS_PANEL_REPLAY(dev_priv)))
> > > +		return;
> > > +
> > > +	drm_dp_dpcd_readb(&intel_dp->aux, DP_PANEL_REPLAY_CAP,
> > &pr_dpcd);
> > > +
> > > +	if (!(pr_dpcd & PANEL_REPLAY_SUPPORT)) {
> > > +		drm_dbg_kms(&dev_priv->drm,
> > > +			    "Panel replay is not supported by panel\n");
> > > +		return;
> > > +	}
> > > +
> > > +	drm_dbg_kms(&dev_priv->drm,
> > > +		    "Panel replay is supported by panel\n");
> > > +	intel_dp->psr.sink_panel_replay_support = true; }
> > > +
> > >  /**
> > >   * intel_psr_init - Init basic PSR work and mutex.
> > >   * @intel_dp: Intel DP
> > > @@ -2183,6 +2229,8 @@ void intel_psr_init(struct intel_dp *intel_dp)
> > >  	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
> > >  	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> > > 
> > > +	intel_panel_replay_init(intel_dp);
> > > +
> > >  	if (!HAS_PSR(dev_priv))
> > >  		return;
> > 
> > mutex initialization is not executed, workers not initialized...
> > please go more carefully trough every PSR function and check what are the
> > panel replay implications
> 
> Tried to double check once more.
> Currently as per bspec and dp 2.0 spec did not see any dependency with PSR.
> Not sure if we really need worker thread to enter in panel replay mode unlike psr where we need to wait for few idle frames.
> Any suggestion/input will be helpful here.
> 
> Regards,
> Animesh
> 
> > 
> > > 
> > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.h
> > > b/drivers/gpu/drm/i915/display/intel_psr.h
> > > index facffbacd357..c9d1c1f0b470 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_psr.h
> > > +++ b/drivers/gpu/drm/i915/display/intel_psr.h
> > > @@ -32,6 +32,7 @@ void intel_psr_flush(struct drm_i915_private *dev_priv,
> > >  		     unsigned frontbuffer_bits,
> > >  		     enum fb_op_origin origin);
> > >  void intel_psr_init(struct intel_dp *intel_dp);
> > > +void intel_panel_replay_init(struct intel_dp *intel_dp);
> > >  void intel_psr_compute_config(struct intel_dp *intel_dp,
> > >  			      struct intel_crtc_state *crtc_state,
> > >  			      struct drm_connector_state *conn_state); @@ -
> > 52,5 +53,7 @@
> > > void intel_psr2_disable_plane_sel_fetch(struct intel_plane *plane,
> > >  					const struct intel_crtc_state
> > *crtc_state);  void
> > > intel_psr_pause(struct intel_dp *intel_dp);  void
> > > intel_psr_resume(struct intel_dp *intel_dp);
> > > +void intel_panel_replay_compute_config(struct intel_dp *intel_dp,
> > > +				       struct intel_crtc_state *crtc_state);
> > > 
> > >  #endif /* __INTEL_PSR_H__ */
>
Manna, Animesh Jan. 4, 2022, 4:44 p.m. UTC | #6
> -----Original Message-----
> From: Souza, Jose <jose.souza@intel.com>
> Sent: Tuesday, January 4, 2022 9:25 PM
> To: dri-devel@lists.freedesktop.org; Manna, Animesh
> <animesh.manna@intel.com>; intel-gfx@lists.freedesktop.org
> Cc: Mun, Gwan-gyeong <gwan-gyeong.mun@intel.com>; Nikula, Jani
> <jani.nikula@intel.com>; Kahola, Mika <mika.kahola@intel.com>; Navare,
> Manasi D <manasi.d.navare@intel.com>
> Subject: Re: [PATCH v3 3/5] drm/i915/panelreplay: Initializaton and compute
> config for panel replay
> 
> On Tue, 2022-01-04 at 21:21 +0530, Manna, Animesh wrote:
> > Hi,
> >
> > > -----Original Message-----
> > > From: Souza, Jose <jose.souza@intel.com>
> > > Sent: Wednesday, November 24, 2021 1:19 AM
> > > To: dri-devel@lists.freedesktop.org; Manna, Animesh
> > > <animesh.manna@intel.com>; intel-gfx@lists.freedesktop.org
> > > Cc: Mun, Gwan-gyeong <gwan-gyeong.mun@intel.com>; Nikula, Jani
> > > <jani.nikula@intel.com>; Kahola, Mika <mika.kahola@intel.com>;
> > > Navare, Manasi D <manasi.d.navare@intel.com>
> > > Subject: Re: [PATCH v3 3/5] drm/i915/panelreplay: Initializaton and
> > > compute config for panel replay
> > >
> > > On Sun, 2021-10-10 at 17:40 +0530, Animesh Manna wrote:
> > > > As panel replay feature similar to PSR feature of EDP panel, so
> > > > currently utilized existing psr framework for panel replay.
> > > >
> > > > v1: RFC version.
> > > > v2: optimized code, pr_enabled and pr_dpcd variable removed.
> > > > [Jose]
> > > > v3:
> > > > - code comments improved. [Jani]
> > > > - dpcd_readb used instead of dpcd_read. [Jani]
> > > > - panel-repaplay init/compute functions moved inside respective
> > > > psr function. [Jani]
> > > >
> > > > Signed-off-by: Animesh Manna <animesh.manna@intel.com>
> > > > ---
> > > >  .../drm/i915/display/intel_display_types.h    |  2 +
> > > >  drivers/gpu/drm/i915/display/intel_dp.c       | 43 +++++++++++++----
> > > >  drivers/gpu/drm/i915/display/intel_psr.c      | 48 +++++++++++++++++++
> > > >  drivers/gpu/drm/i915/display/intel_psr.h      |  3 ++
> > > >  4 files changed, 87 insertions(+), 9 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h
> > > > b/drivers/gpu/drm/i915/display/intel_display_types.h
> > > > index 39e11eaec1a3..48f7d676ed2c 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> > > > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> > > > @@ -1070,6 +1070,7 @@ struct intel_crtc_state {
> > > >  	bool req_psr2_sdp_prior_scanline;
> > > >  	u32 dc3co_exitline;
> > > >  	u16 su_y_granularity;
> > > > +	bool has_panel_replay;
> > >
> > > We can drop this and reuse current ones ones, see bellow.
> > >
> > > >  	struct drm_dp_vsc_sdp psr_vsc;
> > > >
> > > >  	/*
> > > > @@ -1531,6 +1532,7 @@ struct intel_psr {
> > > >  	bool irq_aux_error;
> > > >  	u16 su_w_granularity;
> > > >  	u16 su_y_granularity;
> > > > +	bool sink_panel_replay_support;
> > >
> > > move this closer to has_psr and set both when it is panel replay.
> > > otherwise psr functions will not be executed for panel replay, see CAN_PSR().
> >
> > AFIU Panel replay do not have any dependency with PSR.
> > So that’s why I have created separate function for panel replay which is doing
> similar thing whatever needed for panel replay.
> > For example intel_panel_replay_compute_config() can be independent of
> intel_psr_compute_config().
> > Do you see any dependency with PSR for panel replay?
> 
> There is no dependency but panel replay is PSR for DP so we should re-use PSR
> code as much as possible.
> 
> eDP + sink_support = PSR
> DP + sink_support = panel replay
> 
> So we can reuse has_psr and all other stuff.

Not sure what to reuse from psr other than selective update part which will not enable on dg2 for panel replay.
Will check further on this.

Regards,
Animesh
> 
> >
> > >
> > > >  	u32 dc3co_exitline;
> > > >  	u32 dc3co_exit_delay;
> > > >  	struct delayed_work dc3co_work;
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
> > > > b/drivers/gpu/drm/i915/display/intel_dp.c
> > > > index 10fda20a5bd8..f58a7b72be14 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > > > @@ -1587,12 +1587,22 @@ static void
> > > intel_dp_compute_vsc_colorimetry(const struct intel_crtc_state *crtc
> > > >  	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> > > >  	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> > > >
> > > > -	/*
> > > > -	 * Prepare VSC Header for SU as per DP 1.4 spec, Table 2-118
> > > > -	 * VSC SDP supporting 3D stereo, PSR2, and Pixel Encoding/
> > > > -	 * Colorimetry Format indication.
> > > > -	 */
> > > > -	vsc->revision = 0x5;
> > > > +	if (crtc_state->has_panel_replay) {
> > > > +		/*
> > > > +		 * Prepare VSC Header for SU as per DP 2.0 spec, Table 2-223
> > > > +		 * VSC SDP supporting 3D stereo, Panel Replay, and Pixel
> > > > +		 * Encoding/Colorimetry Format indication.
> > > > +		 */
> > > > +		vsc->revision = 0x7;
> > > > +	} else {
> > > > +		/*
> > > > +		 * Prepare VSC Header for SU as per DP 1.4 spec, Table 2-118
> > > > +		 * VSC SDP supporting 3D stereo, PSR2, and Pixel Encoding/
> > > > +		 * Colorimetry Format indication.
> > > > +		 */
> > > > +		vsc->revision = 0x5;
> > > > +	}
> > > > +
> > > >  	vsc->length = 0x13;
> > > >
> > > >  	/* DP 1.4a spec, Table 2-120 */
> > > > @@ -1701,6 +1711,21 @@ void intel_dp_compute_psr_vsc_sdp(struct
> > > intel_dp *intel_dp,
> > > >  			vsc->revision = 0x4;
> > > >  			vsc->length = 0xe;
> > > >  		}
> > > > +	} else if (intel_dp->psr.enabled && !intel_dp_is_edp(intel_dp)) {
> > > > +		if (intel_dp->psr.colorimetry_support &&
> > > > +		    intel_dp_needs_vsc_sdp(crtc_state, conn_state)) {
> > > > +			/* [Panel Replay with colorimetry info] */
> > > > +			intel_dp_compute_vsc_colorimetry(crtc_state,
> > > conn_state,
> > > > +							 vsc);
> > > > +		} else {
> > > > +			/*
> > > > +			 * [Panel Replay without colorimetry info]
> > > > +			 * Prepare VSC Header for SU as per DP 2.0 spec, Table
> > > 2-223
> > > > +			 * VSC SDP supporting 3D stereo + Panel Replay.
> > > > +			 */
> > > > +			vsc->revision = 0x6;
> > > > +			vsc->length = 0x10;
> > > > +		}
> > > >  	} else {
> > > >  		/*
> > > >  		 * [PSR1]
> > > > @@ -2749,10 +2774,10 @@ static ssize_t intel_dp_vsc_sdp_pack(const
> > > > struct
> > > drm_dp_vsc_sdp *vsc,
> > > >  	sdp->sdp_header.HB3 = vsc->length; /* Number of Valid Data Bytes
> > > > */
> > > >
> > > >  	/*
> > > > -	 * Only revision 0x5 supports Pixel Encoding/Colorimetry Format as
> > > > -	 * per DP 1.4a spec.
> > > > +	 * Revision 0x5 and 0x7 supports Pixel Encoding/Colorimetry Format as
> > > > +	 * per DP 1.4a spec and DP 2.0 spec respectively.
> > > >  	 */
> > > > -	if (vsc->revision != 0x5)
> > > > +	if (vsc->revision != 0x5 || vsc->revision != 0x7)
> > > >  		goto out;
> > > >
> > > >  	/* VSC SDP Payload for DB16 through DB18 */ diff --git
> > > > a/drivers/gpu/drm/i915/display/intel_psr.c
> > > > b/drivers/gpu/drm/i915/display/intel_psr.c
> > > > index 7a205fd5023b..91c2efe2f3ad 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_psr.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> > > > @@ -933,6 +933,21 @@ static bool intel_psr2_config_valid(struct
> > > > intel_dp
> > > *intel_dp,
> > > >  	return true;
> > > >  }
> > > >
> > > > +void intel_panel_replay_compute_config(struct intel_dp *intel_dp,
> > > > +				       struct intel_crtc_state *crtc_state) {
> > > > +	struct drm_i915_private *i915 = dp_to_i915(intel_dp);
> > > > +
> > > > +	if (!intel_dp->psr.sink_panel_replay_support)
> > > > +		return;
> > > > +
> > > > +	crtc_state->has_panel_replay = true;
> > > > +	crtc_state->infoframes.enable |=
> > > > +intel_hdmi_infoframe_enable(DP_SDP_VSC);
> > > > +
> > > > +	if (HAS_PSR2_SEL_FETCH(i915))
> > > > +		intel_psr2_sel_fetch_config_valid(intel_dp, crtc_state); }
> > > > +
> > > >  void intel_psr_compute_config(struct intel_dp *intel_dp,
> > > >  			      struct intel_crtc_state *crtc_state,
> > > >  			      struct drm_connector_state *conn_state) @@ -
> > > 942,6 +957,8 @@
> > > > void intel_psr_compute_config(struct intel_dp *intel_dp,
> > > >  		&crtc_state->hw.adjusted_mode;
> > > >  	int psr_setup_time;
> > > >
> > > > +	intel_panel_replay_compute_config(intel_dp, crtc_state);
> > >
> > > have you checked if the other PSR are needed for panel replay? what
> > > about the
> > > psr2 checks? when using panel replay selective update some
> > > additional tests will be needed regarding granularity...
> >
> > As mentioned above I understood panel replay do not have any dependency
> with PSR.
> > Will not target panel replay selective update for dg2.
> >
> > >
> > > > +
> > > >  	/*
> > > >  	 * Current PSR panels dont work reliably with VRR enabled
> > > >  	 * So if VRR is enabled, do not enable PSR.
> > > > @@ -2170,6 +2187,35 @@ void intel_psr_flush(struct
> > > > drm_i915_private
> > > *dev_priv,
> > > >  	}
> > > >  }
> > > >
> > > > +/**
> > > > + * intel_panel_replay_init - Check for sink and source capability.
> > > > + * @intel_dp: Intel DP
> > > > + *
> > > > + * This function is called after the initializing connector.
> > > > + * (the initializing of connector treats the handling of
> > > > +connector
> > > > +capabilities)
> > > > + * And it initializes basic panel replay stuff for each DP Encoder.
> > > > + */
> > > > +void intel_panel_replay_init(struct intel_dp *intel_dp) {
> > > > +	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> > > > +	u8 pr_dpcd = 0;
> > > > +
> > > > +	if (!(HAS_DP20(dev_priv) && HAS_PANEL_REPLAY(dev_priv)))
> > > > +		return;
> > > > +
> > > > +	drm_dp_dpcd_readb(&intel_dp->aux, DP_PANEL_REPLAY_CAP,
> > > &pr_dpcd);
> > > > +
> > > > +	if (!(pr_dpcd & PANEL_REPLAY_SUPPORT)) {
> > > > +		drm_dbg_kms(&dev_priv->drm,
> > > > +			    "Panel replay is not supported by panel\n");
> > > > +		return;
> > > > +	}
> > > > +
> > > > +	drm_dbg_kms(&dev_priv->drm,
> > > > +		    "Panel replay is supported by panel\n");
> > > > +	intel_dp->psr.sink_panel_replay_support = true; }
> > > > +
> > > >  /**
> > > >   * intel_psr_init - Init basic PSR work and mutex.
> > > >   * @intel_dp: Intel DP
> > > > @@ -2183,6 +2229,8 @@ void intel_psr_init(struct intel_dp *intel_dp)
> > > >  	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
> > > >  	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> > > >
> > > > +	intel_panel_replay_init(intel_dp);
> > > > +
> > > >  	if (!HAS_PSR(dev_priv))
> > > >  		return;
> > >
> > > mutex initialization is not executed, workers not initialized...
> > > please go more carefully trough every PSR function and check what
> > > are the panel replay implications
> >
> > Tried to double check once more.
> > Currently as per bspec and dp 2.0 spec did not see any dependency with PSR.
> > Not sure if we really need worker thread to enter in panel replay mode unlike
> psr where we need to wait for few idle frames.
> > Any suggestion/input will be helpful here.
> >
> > Regards,
> > Animesh
> >
> > >
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.h
> > > > b/drivers/gpu/drm/i915/display/intel_psr.h
> > > > index facffbacd357..c9d1c1f0b470 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_psr.h
> > > > +++ b/drivers/gpu/drm/i915/display/intel_psr.h
> > > > @@ -32,6 +32,7 @@ void intel_psr_flush(struct drm_i915_private
> *dev_priv,
> > > >  		     unsigned frontbuffer_bits,
> > > >  		     enum fb_op_origin origin);  void intel_psr_init(struct
> > > > intel_dp *intel_dp);
> > > > +void intel_panel_replay_init(struct intel_dp *intel_dp);
> > > >  void intel_psr_compute_config(struct intel_dp *intel_dp,
> > > >  			      struct intel_crtc_state *crtc_state,
> > > >  			      struct drm_connector_state *conn_state); @@ -
> > > 52,5 +53,7 @@
> > > > void intel_psr2_disable_plane_sel_fetch(struct intel_plane *plane,
> > > >  					const struct intel_crtc_state
> > > *crtc_state);  void
> > > > intel_psr_pause(struct intel_dp *intel_dp);  void
> > > > intel_psr_resume(struct intel_dp *intel_dp);
> > > > +void intel_panel_replay_compute_config(struct intel_dp *intel_dp,
> > > > +				       struct intel_crtc_state *crtc_state);
> > > >
> > > >  #endif /* __INTEL_PSR_H__ */
> >
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
index 39e11eaec1a3..48f7d676ed2c 100644
--- a/drivers/gpu/drm/i915/display/intel_display_types.h
+++ b/drivers/gpu/drm/i915/display/intel_display_types.h
@@ -1070,6 +1070,7 @@  struct intel_crtc_state {
 	bool req_psr2_sdp_prior_scanline;
 	u32 dc3co_exitline;
 	u16 su_y_granularity;
+	bool has_panel_replay;
 	struct drm_dp_vsc_sdp psr_vsc;
 
 	/*
@@ -1531,6 +1532,7 @@  struct intel_psr {
 	bool irq_aux_error;
 	u16 su_w_granularity;
 	u16 su_y_granularity;
+	bool sink_panel_replay_support;
 	u32 dc3co_exitline;
 	u32 dc3co_exit_delay;
 	struct delayed_work dc3co_work;
diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
index 10fda20a5bd8..f58a7b72be14 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -1587,12 +1587,22 @@  static void intel_dp_compute_vsc_colorimetry(const struct intel_crtc_state *crtc
 	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
 	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
 
-	/*
-	 * Prepare VSC Header for SU as per DP 1.4 spec, Table 2-118
-	 * VSC SDP supporting 3D stereo, PSR2, and Pixel Encoding/
-	 * Colorimetry Format indication.
-	 */
-	vsc->revision = 0x5;
+	if (crtc_state->has_panel_replay) {
+		/*
+		 * Prepare VSC Header for SU as per DP 2.0 spec, Table 2-223
+		 * VSC SDP supporting 3D stereo, Panel Replay, and Pixel
+		 * Encoding/Colorimetry Format indication.
+		 */
+		vsc->revision = 0x7;
+	} else {
+		/*
+		 * Prepare VSC Header for SU as per DP 1.4 spec, Table 2-118
+		 * VSC SDP supporting 3D stereo, PSR2, and Pixel Encoding/
+		 * Colorimetry Format indication.
+		 */
+		vsc->revision = 0x5;
+	}
+
 	vsc->length = 0x13;
 
 	/* DP 1.4a spec, Table 2-120 */
@@ -1701,6 +1711,21 @@  void intel_dp_compute_psr_vsc_sdp(struct intel_dp *intel_dp,
 			vsc->revision = 0x4;
 			vsc->length = 0xe;
 		}
+	} else if (intel_dp->psr.enabled && !intel_dp_is_edp(intel_dp)) {
+		if (intel_dp->psr.colorimetry_support &&
+		    intel_dp_needs_vsc_sdp(crtc_state, conn_state)) {
+			/* [Panel Replay with colorimetry info] */
+			intel_dp_compute_vsc_colorimetry(crtc_state, conn_state,
+							 vsc);
+		} else {
+			/*
+			 * [Panel Replay without colorimetry info]
+			 * Prepare VSC Header for SU as per DP 2.0 spec, Table 2-223
+			 * VSC SDP supporting 3D stereo + Panel Replay.
+			 */
+			vsc->revision = 0x6;
+			vsc->length = 0x10;
+		}
 	} else {
 		/*
 		 * [PSR1]
@@ -2749,10 +2774,10 @@  static ssize_t intel_dp_vsc_sdp_pack(const struct drm_dp_vsc_sdp *vsc,
 	sdp->sdp_header.HB3 = vsc->length; /* Number of Valid Data Bytes */
 
 	/*
-	 * Only revision 0x5 supports Pixel Encoding/Colorimetry Format as
-	 * per DP 1.4a spec.
+	 * Revision 0x5 and 0x7 supports Pixel Encoding/Colorimetry Format as
+	 * per DP 1.4a spec and DP 2.0 spec respectively.
 	 */
-	if (vsc->revision != 0x5)
+	if (vsc->revision != 0x5 || vsc->revision != 0x7)
 		goto out;
 
 	/* VSC SDP Payload for DB16 through DB18 */
diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c
index 7a205fd5023b..91c2efe2f3ad 100644
--- a/drivers/gpu/drm/i915/display/intel_psr.c
+++ b/drivers/gpu/drm/i915/display/intel_psr.c
@@ -933,6 +933,21 @@  static bool intel_psr2_config_valid(struct intel_dp *intel_dp,
 	return true;
 }
 
+void intel_panel_replay_compute_config(struct intel_dp *intel_dp,
+				       struct intel_crtc_state *crtc_state)
+{
+	struct drm_i915_private *i915 = dp_to_i915(intel_dp);
+
+	if (!intel_dp->psr.sink_panel_replay_support)
+		return;
+
+	crtc_state->has_panel_replay = true;
+	crtc_state->infoframes.enable |= intel_hdmi_infoframe_enable(DP_SDP_VSC);
+
+	if (HAS_PSR2_SEL_FETCH(i915))
+		intel_psr2_sel_fetch_config_valid(intel_dp, crtc_state);
+}
+
 void intel_psr_compute_config(struct intel_dp *intel_dp,
 			      struct intel_crtc_state *crtc_state,
 			      struct drm_connector_state *conn_state)
@@ -942,6 +957,8 @@  void intel_psr_compute_config(struct intel_dp *intel_dp,
 		&crtc_state->hw.adjusted_mode;
 	int psr_setup_time;
 
+	intel_panel_replay_compute_config(intel_dp, crtc_state);
+
 	/*
 	 * Current PSR panels dont work reliably with VRR enabled
 	 * So if VRR is enabled, do not enable PSR.
@@ -2170,6 +2187,35 @@  void intel_psr_flush(struct drm_i915_private *dev_priv,
 	}
 }
 
+/**
+ * intel_panel_replay_init - Check for sink and source capability.
+ * @intel_dp: Intel DP
+ *
+ * This function is called after the initializing connector.
+ * (the initializing of connector treats the handling of connector capabilities)
+ * And it initializes basic panel replay stuff for each DP Encoder.
+ */
+void intel_panel_replay_init(struct intel_dp *intel_dp)
+{
+	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
+	u8 pr_dpcd = 0;
+
+	if (!(HAS_DP20(dev_priv) && HAS_PANEL_REPLAY(dev_priv)))
+		return;
+
+	drm_dp_dpcd_readb(&intel_dp->aux, DP_PANEL_REPLAY_CAP, &pr_dpcd);
+
+	if (!(pr_dpcd & PANEL_REPLAY_SUPPORT)) {
+		drm_dbg_kms(&dev_priv->drm,
+			    "Panel replay is not supported by panel\n");
+		return;
+	}
+
+	drm_dbg_kms(&dev_priv->drm,
+		    "Panel replay is supported by panel\n");
+	intel_dp->psr.sink_panel_replay_support = true;
+}
+
 /**
  * intel_psr_init - Init basic PSR work and mutex.
  * @intel_dp: Intel DP
@@ -2183,6 +2229,8 @@  void intel_psr_init(struct intel_dp *intel_dp)
 	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
 	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
 
+	intel_panel_replay_init(intel_dp);
+
 	if (!HAS_PSR(dev_priv))
 		return;
 
diff --git a/drivers/gpu/drm/i915/display/intel_psr.h b/drivers/gpu/drm/i915/display/intel_psr.h
index facffbacd357..c9d1c1f0b470 100644
--- a/drivers/gpu/drm/i915/display/intel_psr.h
+++ b/drivers/gpu/drm/i915/display/intel_psr.h
@@ -32,6 +32,7 @@  void intel_psr_flush(struct drm_i915_private *dev_priv,
 		     unsigned frontbuffer_bits,
 		     enum fb_op_origin origin);
 void intel_psr_init(struct intel_dp *intel_dp);
+void intel_panel_replay_init(struct intel_dp *intel_dp);
 void intel_psr_compute_config(struct intel_dp *intel_dp,
 			      struct intel_crtc_state *crtc_state,
 			      struct drm_connector_state *conn_state);
@@ -52,5 +53,7 @@  void intel_psr2_disable_plane_sel_fetch(struct intel_plane *plane,
 					const struct intel_crtc_state *crtc_state);
 void intel_psr_pause(struct intel_dp *intel_dp);
 void intel_psr_resume(struct intel_dp *intel_dp);
+void intel_panel_replay_compute_config(struct intel_dp *intel_dp,
+				       struct intel_crtc_state *crtc_state);
 
 #endif /* __INTEL_PSR_H__ */