Re: [PATCH v8 2/6] drm: Rename struct edp_vsc_psr to struct dp_sdp

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, 2019-05-21 at 13:14 +0300, Laurent Pinchart wrote:
> Hello Jani,
> 
> On Tue, May 21, 2019 at 09:44:04AM +0300, Jani Nikula wrote:
> > On Mon, 20 May 2019, Gwan-gyeong Mun <gwan-gyeong.mun@xxxxxxxxx>
> > wrote:
> > > VSC SDP Payload for PSR is one of data block type of SDP
> > > (Secondaray Data
> > > Packet). In order to generalize SDP packet structure name, it
> > > renames
> > > struct edp_vsc_psr to struct dp_sdp. And each SDP data blocks
> > > have
> > > different usages, each SDP type has different reserved data
> > > blocks and
> > > Video_Stream_Configuration Extension VESA SDP might use all of
> > > Data Blocks
> > > as Extended INFORFRAME Data Byte. so it makes Data Block
> > > variables as
> > > array type. And it adds comments of details of DB of VSC SDP
> > > Payload
> > > for Pixel Encoding/Colorimetry Format. This comments follows DP
> > > 1.4a spec,
> > > section 2.2.5.7.5, chapter "VSC SDP Payload for Pixel
> > > Encoding/Colorimetry
> > > Format".
> > > 
> > > v7: Addressed review comments from Ville.
> > > 
> > > Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > > Signed-off-by: Gwan-gyeong Mun <gwan-gyeong.mun@xxxxxxxxx>
> > > Reviewed-by: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx
> > > >
> > 
> > Andrzej, Laurent -
> > 
> > Seven versions of the patch and looks like we've failed to loop you
> > in
> > on this. Sorry. May I have your ack on the patch please?
> 
> Please see below for one comment.
> 
> > Is it too much to ask to have this merged via drm-intel along with
> > the
> > rest of the series?
> 
> Provided the potential conflicts are handled I have no issue with
> that.
> 
Andrzej, Laurent -

I am Sorry that I missed you on previous loops.
And thank you for reviewing the series.

Jani,
Thank you for asking the series to Andrzej and Laurent.
> > > ---
> > >  .../drm/bridge/analogix/analogix_dp_core.c    | 12 +++----
> > >  .../drm/bridge/analogix/analogix_dp_core.h    |  2 +-
> > >  .../gpu/drm/bridge/analogix/analogix_dp_reg.c | 10 +++---
> > >  drivers/gpu/drm/i915/intel_psr.c              |  2 +-
> > >  include/drm/drm_dp_helper.h                   | 33
> > > +++++++++++++------
> > >  5 files changed, 36 insertions(+), 23 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> > > b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> > > index 225f5e5dd69b..d1c2659d0cce 100644
> > > --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> > > +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> > > @@ -115,7 +115,7 @@ EXPORT_SYMBOL_GPL(analogix_dp_psr_enabled);
> > >  
> > >  int analogix_dp_enable_psr(struct analogix_dp_device *dp)
> > >  {
> > > -	struct edp_vsc_psr psr_vsc;
> > > +	struct dp_sdp psr_vsc;
> > >  
> > >  	if (!dp->psr_enable)
> > >  		return 0;
> > > @@ -127,8 +127,8 @@ int analogix_dp_enable_psr(struct
> > > analogix_dp_device *dp)
> > >  	psr_vsc.sdp_header.HB2 = 0x2;
> > >  	psr_vsc.sdp_header.HB3 = 0x8;
> > >  
> > > -	psr_vsc.DB0 = 0;
> > > -	psr_vsc.DB1 = EDP_VSC_PSR_STATE_ACTIVE |
> > > EDP_VSC_PSR_CRC_VALUES_VALID;
> > > +	psr_vsc.DB[0] = 0;
> > > +	psr_vsc.DB[1] = EDP_VSC_PSR_STATE_ACTIVE |
> > > EDP_VSC_PSR_CRC_VALUES_VALID;
> > >  
> > >  	return analogix_dp_send_psr_spd(dp, &psr_vsc, true);
> > >  }
> > > @@ -136,7 +136,7 @@ EXPORT_SYMBOL_GPL(analogix_dp_enable_psr);
> > >  
> > >  int analogix_dp_disable_psr(struct analogix_dp_device *dp)
> > >  {
> > > -	struct edp_vsc_psr psr_vsc;
> > > +	struct dp_sdp psr_vsc;
> > >  	int ret;
> > >  
> > >  	if (!dp->psr_enable)
> > > @@ -149,8 +149,8 @@ int analogix_dp_disable_psr(struct
> > > analogix_dp_device *dp)
> > >  	psr_vsc.sdp_header.HB2 = 0x2;
> > >  	psr_vsc.sdp_header.HB3 = 0x8;
> > >  
> > > -	psr_vsc.DB0 = 0;
> > > -	psr_vsc.DB1 = 0;
> > > +	psr_vsc.DB[0] = 0;
> > > +	psr_vsc.DB[1] = 0;
> > >  
> > >  	ret = drm_dp_dpcd_writeb(&dp->aux, DP_SET_POWER,
> > > DP_SET_POWER_D0);
> > >  	if (ret != 1) {
> > > diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
> > > b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
> > > index 769255dc6e99..3e5fe90edf71 100644
> > > --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
> > > +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
> > > @@ -254,7 +254,7 @@ void analogix_dp_enable_scrambling(struct
> > > analogix_dp_device *dp);
> > >  void analogix_dp_disable_scrambling(struct analogix_dp_device
> > > *dp);
> > >  void analogix_dp_enable_psr_crc(struct analogix_dp_device *dp);
> > >  int analogix_dp_send_psr_spd(struct analogix_dp_device *dp,
> > > -			     struct edp_vsc_psr *vsc, bool blocking);
> > > +			     struct dp_sdp *vsc, bool blocking);
> > >  ssize_t analogix_dp_transfer(struct analogix_dp_device *dp,
> > >  			     struct drm_dp_aux_msg *msg);
> > >  
> > > diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
> > > b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
> > > index a5f2763d72e4..f591810ef1be 100644
> > > --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
> > > +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
> > > @@ -1041,7 +1041,7 @@ static ssize_t
> > > analogix_dp_get_psr_status(struct analogix_dp_device *dp)
> > >  }
> > >  
> > >  int analogix_dp_send_psr_spd(struct analogix_dp_device *dp,
> > > -			     struct edp_vsc_psr *vsc, bool blocking)
> > > +			     struct dp_sdp *vsc, bool blocking)
> > >  {
> > >  	unsigned int val;
> > >  	int ret;
> > > @@ -1069,8 +1069,8 @@ int analogix_dp_send_psr_spd(struct
> > > analogix_dp_device *dp,
> > >  	writel(0x5D, dp->reg_base + ANALOGIX_DP_SPD_PB3);
> > >  
> > >  	/* configure DB0 / DB1 values */
> > > -	writel(vsc->DB0, dp->reg_base + ANALOGIX_DP_VSC_SHADOW_DB0);
> > > -	writel(vsc->DB1, dp->reg_base + ANALOGIX_DP_VSC_SHADOW_DB1);
> > > +	writel(vsc->DB[0], dp->reg_base + ANALOGIX_DP_VSC_SHADOW_DB0);
> > > +	writel(vsc->DB[1], dp->reg_base + ANALOGIX_DP_VSC_SHADOW_DB1);
> > >  
> > >  	/* set reuse spd inforframe */
> > >  	val = readl(dp->reg_base + ANALOGIX_DP_VIDEO_CTL_3);
> > > @@ -1092,8 +1092,8 @@ int analogix_dp_send_psr_spd(struct
> > > analogix_dp_device *dp,
> > >  
> > >  	ret = readx_poll_timeout(analogix_dp_get_psr_status, dp,
> > > psr_status,
> > >  		psr_status >= 0 &&
> > > -		((vsc->DB1 && psr_status == DP_PSR_SINK_ACTIVE_RFB) ||
> > > -		(!vsc->DB1 && psr_status == DP_PSR_SINK_INACTIVE)),
> > > 1500,
> > > +		((vsc->DB[1] && psr_status == DP_PSR_SINK_ACTIVE_RFB)
> > > ||
> > > +		(!vsc->DB[1] && 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);
> > > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > > b/drivers/gpu/drm/i915/intel_psr.c
> > > index 2a547a128a37..01ca502099df 100644
> > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > @@ -342,7 +342,7 @@ static void intel_psr_setup_vsc(struct
> > > intel_dp *intel_dp,
> > >  {
> > >  	struct intel_digital_port *intel_dig_port =
> > > dp_to_dig_port(intel_dp);
> > >  	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> > > -	struct edp_vsc_psr psr_vsc;
> > > +	struct dp_sdp psr_vsc;
> > >  
> > >  	if (dev_priv->psr.psr2_enabled) {
> > >  		/* Prepare VSC Header for SU as per EDP 1.4 spec, Table
> > > 6.11 */
> > > diff --git a/include/drm/drm_dp_helper.h
> > > b/include/drm/drm_dp_helper.h
> > > index 97ce790a5b5a..8d7c47e46f2d 100644
> > > --- a/include/drm/drm_dp_helper.h
> > > +++ b/include/drm/drm_dp_helper.h
> > > @@ -1083,17 +1083,30 @@ struct dp_sdp_header {
> > >  #define EDP_SDP_HEADER_VALID_PAYLOAD_BYTES	0x1F
> > >  #define DP_SDP_PPS_HEADER_PAYLOAD_BYTES_MINUS_1 0x7F
> > >  
> > > -struct edp_vsc_psr {
> > > +/**
> > > + * struct dp_sdp - DP secondary data packet
> > > + * @sdp_header: DP secondary data packet header
> > > + * @DB: DP secondaray data packet data blocks
> > > + * VSC SDP Payload for PSR
> > > + * DB[0]: Stereo Interface
> > > + * DB[1]: 0 - PSR State; 1 - Update RFB; 2 - CRC Valid
> > > + * DB[2]: CRC value bits 7:0 of the R or Cr component
> > > + * DB[3]: CRC value bits 15:8 of the R or Cr component
> > > + * DB[4]: CRC value bits 7:0 of the G or Y component
> > > + * DB[5]: CRC value bits 15:8 of the G or Y component
> > > + * DB[6]: CRC value bits 7:0 of the B or Cb component
> > > + * DB[7]: CRC value bits 15:8 of the B or Cb component
> > > + * DB[8] - DB[31]: Reserved
> > > + * VSC SDP Payload for Pixel Encoding/Colorimetry Format
> > > + * DB[0] - DB[15]: Reserved
> > > + * DB[16]: Pixel Encoding and Colorimetry Formats
> > > + * DB[17]: Dynamic Range and Component Bit Depth
> > > + * DB[18]: Content Type
> > > + * DB[19] - DB[31]: Reserved
> > > + */
> > > +struct dp_sdp {
> > >  	struct dp_sdp_header sdp_header;
> > > -	u8 DB0; /* Stereo Interface */
> > > -	u8 DB1; /* 0 - PSR State; 1 - Update RFB; 2 - CRC Valid */
> > > -	u8 DB2; /* CRC value bits 7:0 of the R or Cr component */
> > > -	u8 DB3; /* CRC value bits 15:8 of the R or Cr component */
> > > -	u8 DB4; /* CRC value bits 7:0 of the G or Y component */
> > > -	u8 DB5; /* CRC value bits 15:8 of the G or Y component */
> > > -	u8 DB6; /* CRC value bits 7:0 of the B or Cb component */
> > > -	u8 DB7; /* CRC value bits 15:8 of the B or Cb component */
> > > -	u8 DB8_31[24]; /* Reserved */
> > > +	u8 DB[32];
> 
> While at it, would it make sense to rename DB to db ?
> 
Okay, I'll update DB to db.

> Apart from that,
> 
> Acked-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> 
> > >  } __packed;
> > >  
> > >  #define EDP_VSC_PSR_STATE_ACTIVE	(1<<0)
> > 
> > -- 
> > Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux