| Message ID | 1470901072-17525-2-git-send-email-vathsala.nagaraju@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Aug 11, 2016 at 01:07:50PM +0530, vathsala nagaraju wrote: > Adds Y-co-ordinate support to skl_psr_setup_vsc as > per edp 1.4 spec,table 6-11:VSC SDP HEADER > Extension for psr2 support. > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > Signed-off-by: vathsala nagaraju <vathsala.nagaraju@intel.com> > --- > drivers/gpu/drm/i915/i915_drv.h | 2 ++ > drivers/gpu/drm/i915/intel_dp.c | 22 ++++++++++++++++++++++ > drivers/gpu/drm/i915/intel_psr.c | 13 ++++++++++++- > include/drm/drm_dp_helper.h | 5 ++++- > 4 files changed, 40 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 7f2754a..79ce64f 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -1022,6 +1022,8 @@ struct i915_psr { > bool psr2_support; > bool aux_frame_sync; > bool link_standby; > + bool y_cord_support; > + bool colorimetry_support; > }; > > enum intel_pch { > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index 364db90..19e9ecf 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -3439,6 +3439,28 @@ intel_edp_init_dpcd(struct intel_dp *intel_dp) > dev_priv->psr.psr2_support = dev_priv->psr.aux_frame_sync; > DRM_DEBUG_KMS("PSR2 %s on sink", > dev_priv->psr.psr2_support ? "supported" : "not supported"); > + > + if (dev_priv->psr.psr2_support) { > + uint8_t psr_caps, dprx; > + > + /*check if panel supports Y-Cordinate*/ > + drm_dp_dpcd_readb(&intel_dp->aux, > + DP_PSR_CAPS, > + &psr_caps); intel_dp->edp_dpcd[1] We should probably add something resembling dp_link_status() for each DPCD chunk we cache, to make it less confusing to use them. > + if (psr_caps & DP_PSR_Y_COORDINATE) > + dev_priv->psr.y_cord_support = true; > + else > + dev_priv->psr.y_cord_support = false; > + /* check for COLORIMETRY SUPPORT */ > + drm_dp_dpcd_readb(&intel_dp->aux, > + DPRX_FEATURE_ENUMERATION_LIST, > + &dprx); > + if (dprx & VSC_SDP_EXT_FOR_COLORIMETRY_SUPPORTED) > + dev_priv->psr.colorimetry_support = true; > + else > + dev_priv->psr.colorimetry_support = false; > + } > + > } > > /* Read the eDP Display control capabilities registers */ > diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c > index 59a21c9..76a630b 100644 > --- a/drivers/gpu/drm/i915/intel_psr.c > +++ b/drivers/gpu/drm/i915/intel_psr.c > @@ -122,13 +122,24 @@ static void vlv_psr_setup_vsc(struct intel_dp *intel_dp) > static void skl_psr_setup_su_vsc(struct intel_dp *intel_dp) > { > struct edp_vsc_psr psr_vsc; > + struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); > + struct drm_device *dev = intel_dig_port->base.base.dev; > + struct drm_i915_private *dev_priv = to_i915(dev); > > /* Prepare VSC Header for SU as per EDP 1.4 spec, Table 6.11 */ > memset(&psr_vsc, 0, sizeof(psr_vsc)); > psr_vsc.sdp_header.HB0 = 0; > psr_vsc.sdp_header.HB1 = 0x7; > psr_vsc.sdp_header.HB2 = 0x3; > - psr_vsc.sdp_header.HB3 = 0xb; > + psr_vsc.sdp_header.HB3 = 0xc; > + if (dev_priv->psr.y_cord_support && > + dev_priv->psr.colorimetry_support) { > + psr_vsc.sdp_header.HB2 = 0x5; > + psr_vsc.sdp_header.HB3 = 0x13; > + } else { > + psr_vsc.sdp_header.HB2 = 0x4; > + psr_vsc.sdp_header.HB3 = 0xe; > + } That looks bogus. Why do we claim to have colorimetry data but then don't fill it out? Also you're not setting the actual y coordinate stuff anywhere, so why would we want to indicate that we support it? > intel_psr_write_vsc(intel_dp, &psr_vsc); > } > > diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h > index 63b8bd5..3d875c0 100644 > --- a/include/drm/drm_dp_helper.h > +++ b/include/drm/drm_dp_helper.h > @@ -194,7 +194,7 @@ > # define DP_PSR_SETUP_TIME_0 (6 << 1) > # define DP_PSR_SETUP_TIME_MASK (7 << 1) > # define DP_PSR_SETUP_TIME_SHIFT 1 > - > +# define DP_PSR_Y_COORDINATE (1 << 4) > /* > * 0x80-0x8f describe downstream port capabilities, but there are two layouts > * based on whether DP_DETAILED_CAP_INFO_AVAILABLE was set. If it was not, > @@ -640,6 +640,9 @@ struct edp_sdp_header { > #define EDP_SDP_HEADER_REVISION_MASK 0x1F > #define EDP_SDP_HEADER_VALID_PAYLOAD_BYTES 0x1F > > +#define DPRX_FEATURE_ENUMERATION_LIST 0x02210 > +#define VSC_SDP_EXT_FOR_COLORIMETRY_SUPPORTED (1 << 3) > + > struct edp_vsc_psr { > struct edp_sdp_header sdp_header; > u8 DB0; /* Stereo Interface */ > -- > 2.7.4 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Thursday 11 August 2016 01:30 PM, Ville Syrjälä wrote: > On Thu, Aug 11, 2016 at 01:07:50PM +0530, vathsala nagaraju wrote: >> Adds Y-co-ordinate support to skl_psr_setup_vsc as >> per edp 1.4 spec,table 6-11:VSC SDP HEADER >> Extension for psr2 support. >> >> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> >> Signed-off-by: vathsala nagaraju <vathsala.nagaraju@intel.com> >> --- >> drivers/gpu/drm/i915/i915_drv.h | 2 ++ >> drivers/gpu/drm/i915/intel_dp.c | 22 ++++++++++++++++++++++ >> drivers/gpu/drm/i915/intel_psr.c | 13 ++++++++++++- >> include/drm/drm_dp_helper.h | 5 ++++- >> 4 files changed, 40 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h >> index 7f2754a..79ce64f 100644 >> --- a/drivers/gpu/drm/i915/i915_drv.h >> +++ b/drivers/gpu/drm/i915/i915_drv.h >> @@ -1022,6 +1022,8 @@ struct i915_psr { >> bool psr2_support; >> bool aux_frame_sync; >> bool link_standby; >> + bool y_cord_support; >> + bool colorimetry_support; >> }; >> >> enum intel_pch { >> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c >> index 364db90..19e9ecf 100644 >> --- a/drivers/gpu/drm/i915/intel_dp.c >> +++ b/drivers/gpu/drm/i915/intel_dp.c >> @@ -3439,6 +3439,28 @@ intel_edp_init_dpcd(struct intel_dp *intel_dp) >> dev_priv->psr.psr2_support = dev_priv->psr.aux_frame_sync; >> DRM_DEBUG_KMS("PSR2 %s on sink", >> dev_priv->psr.psr2_support ? "supported" : "not supported"); >> + >> + if (dev_priv->psr.psr2_support) { >> + uint8_t psr_caps, dprx; >> + >> + /*check if panel supports Y-Cordinate*/ >> + drm_dp_dpcd_readb(&intel_dp->aux, >> + DP_PSR_CAPS, >> + &psr_caps); > intel_dp->edp_dpcd[1] > > We should probably add something resembling dp_link_status() for each > DPCD chunk we cache, to make it less confusing to use them. > >> + if (psr_caps & DP_PSR_Y_COORDINATE) >> + dev_priv->psr.y_cord_support = true; >> + else >> + dev_priv->psr.y_cord_support = false; >> + /* check for COLORIMETRY SUPPORT */ >> + drm_dp_dpcd_readb(&intel_dp->aux, >> + DPRX_FEATURE_ENUMERATION_LIST, >> + &dprx); >> + if (dprx & VSC_SDP_EXT_FOR_COLORIMETRY_SUPPORTED) >> + dev_priv->psr.colorimetry_support = true; >> + else >> + dev_priv->psr.colorimetry_support = false; >> + } >> + >> } >> >> /* Read the eDP Display control capabilities registers */ >> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c >> index 59a21c9..76a630b 100644 >> --- a/drivers/gpu/drm/i915/intel_psr.c >> +++ b/drivers/gpu/drm/i915/intel_psr.c >> @@ -122,13 +122,24 @@ static void vlv_psr_setup_vsc(struct intel_dp *intel_dp) >> static void skl_psr_setup_su_vsc(struct intel_dp *intel_dp) >> { >> struct edp_vsc_psr psr_vsc; >> + struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); >> + struct drm_device *dev = intel_dig_port->base.base.dev; >> + struct drm_i915_private *dev_priv = to_i915(dev); >> >> /* Prepare VSC Header for SU as per EDP 1.4 spec, Table 6.11 */ >> memset(&psr_vsc, 0, sizeof(psr_vsc)); >> psr_vsc.sdp_header.HB0 = 0; >> psr_vsc.sdp_header.HB1 = 0x7; >> psr_vsc.sdp_header.HB2 = 0x3; >> - psr_vsc.sdp_header.HB3 = 0xb; >> + psr_vsc.sdp_header.HB3 = 0xc; >> + if (dev_priv->psr.y_cord_support && >> + dev_priv->psr.colorimetry_support) { >> + psr_vsc.sdp_header.HB2 = 0x5; >> + psr_vsc.sdp_header.HB3 = 0x13; >> + } else { >> + psr_vsc.sdp_header.HB2 = 0x4; >> + psr_vsc.sdp_header.HB3 = 0xe; >> + } > That looks bogus. Why do we claim to have colorimetry data but > then don't fill it out? HB2 to be set 04 or 05 04h = 3D stereo + PSR/PSR2 + Y-coordinate. 05h = 3D stereo- + PSR/PSR2 + Y-coordinate + Pixel Encoding/Colorimetry Format As of now it's defaulting to 0x4, will correct it. > > Also you're not setting the actual y coordinate stuff anywhere, so why > would we want to indicate that we support it? > Bspec says to set CHICKEN_TRANS_EDP(0x420cc) bit 15 if Y coordinate is supported. it set in patch 2. >> intel_psr_write_vsc(intel_dp, &psr_vsc); >> } >> >> diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h >> index 63b8bd5..3d875c0 100644 >> --- a/include/drm/drm_dp_helper.h >> +++ b/include/drm/drm_dp_helper.h >> @@ -194,7 +194,7 @@ >> # define DP_PSR_SETUP_TIME_0 (6 << 1) >> # define DP_PSR_SETUP_TIME_MASK (7 << 1) >> # define DP_PSR_SETUP_TIME_SHIFT 1 >> - >> +# define DP_PSR_Y_COORDINATE (1 << 4) >> /* >> * 0x80-0x8f describe downstream port capabilities, but there are two layouts >> * based on whether DP_DETAILED_CAP_INFO_AVAILABLE was set. If it was not, >> @@ -640,6 +640,9 @@ struct edp_sdp_header { >> #define EDP_SDP_HEADER_REVISION_MASK 0x1F >> #define EDP_SDP_HEADER_VALID_PAYLOAD_BYTES 0x1F >> >> +#define DPRX_FEATURE_ENUMERATION_LIST 0x02210 >> +#define VSC_SDP_EXT_FOR_COLORIMETRY_SUPPORTED (1 << 3) >> + >> struct edp_vsc_psr { >> struct edp_sdp_header sdp_header; >> u8 DB0; /* Stereo Interface */ >> -- >> 2.7.4 >> >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Fri, Aug 12, 2016 at 10:48:12AM +0530, vathsala nagaraju wrote: > On Thursday 11 August 2016 01:30 PM, Ville Syrjälä wrote: > > On Thu, Aug 11, 2016 at 01:07:50PM +0530, vathsala nagaraju wrote: > >> Adds Y-co-ordinate support to skl_psr_setup_vsc as > >> per edp 1.4 spec,table 6-11:VSC SDP HEADER > >> Extension for psr2 support. > >> > >> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > >> Signed-off-by: vathsala nagaraju <vathsala.nagaraju@intel.com> > >> --- > >> drivers/gpu/drm/i915/i915_drv.h | 2 ++ > >> drivers/gpu/drm/i915/intel_dp.c | 22 ++++++++++++++++++++++ > >> drivers/gpu/drm/i915/intel_psr.c | 13 ++++++++++++- > >> include/drm/drm_dp_helper.h | 5 ++++- > >> 4 files changed, 40 insertions(+), 2 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > >> index 7f2754a..79ce64f 100644 > >> --- a/drivers/gpu/drm/i915/i915_drv.h > >> +++ b/drivers/gpu/drm/i915/i915_drv.h > >> @@ -1022,6 +1022,8 @@ struct i915_psr { > >> bool psr2_support; > >> bool aux_frame_sync; > >> bool link_standby; > >> + bool y_cord_support; > >> + bool colorimetry_support; > >> }; > >> > >> enum intel_pch { > >> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > >> index 364db90..19e9ecf 100644 > >> --- a/drivers/gpu/drm/i915/intel_dp.c > >> +++ b/drivers/gpu/drm/i915/intel_dp.c > >> @@ -3439,6 +3439,28 @@ intel_edp_init_dpcd(struct intel_dp *intel_dp) > >> dev_priv->psr.psr2_support = dev_priv->psr.aux_frame_sync; > >> DRM_DEBUG_KMS("PSR2 %s on sink", > >> dev_priv->psr.psr2_support ? "supported" : "not supported"); > >> + > >> + if (dev_priv->psr.psr2_support) { > >> + uint8_t psr_caps, dprx; > >> + > >> + /*check if panel supports Y-Cordinate*/ > >> + drm_dp_dpcd_readb(&intel_dp->aux, > >> + DP_PSR_CAPS, > >> + &psr_caps); > > intel_dp->edp_dpcd[1] > > > > We should probably add something resembling dp_link_status() for each > > DPCD chunk we cache, to make it less confusing to use them. > > > >> + if (psr_caps & DP_PSR_Y_COORDINATE) > >> + dev_priv->psr.y_cord_support = true; > >> + else > >> + dev_priv->psr.y_cord_support = false; > >> + /* check for COLORIMETRY SUPPORT */ > >> + drm_dp_dpcd_readb(&intel_dp->aux, > >> + DPRX_FEATURE_ENUMERATION_LIST, > >> + &dprx); > >> + if (dprx & VSC_SDP_EXT_FOR_COLORIMETRY_SUPPORTED) > >> + dev_priv->psr.colorimetry_support = true; > >> + else > >> + dev_priv->psr.colorimetry_support = false; > >> + } > >> + > >> } > >> > >> /* Read the eDP Display control capabilities registers */ > >> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c > >> index 59a21c9..76a630b 100644 > >> --- a/drivers/gpu/drm/i915/intel_psr.c > >> +++ b/drivers/gpu/drm/i915/intel_psr.c > >> @@ -122,13 +122,24 @@ static void vlv_psr_setup_vsc(struct intel_dp *intel_dp) > >> static void skl_psr_setup_su_vsc(struct intel_dp *intel_dp) > >> { > >> struct edp_vsc_psr psr_vsc; > >> + struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); > >> + struct drm_device *dev = intel_dig_port->base.base.dev; > >> + struct drm_i915_private *dev_priv = to_i915(dev); > >> > >> /* Prepare VSC Header for SU as per EDP 1.4 spec, Table 6.11 */ > >> memset(&psr_vsc, 0, sizeof(psr_vsc)); > >> psr_vsc.sdp_header.HB0 = 0; > >> psr_vsc.sdp_header.HB1 = 0x7; > >> psr_vsc.sdp_header.HB2 = 0x3; > >> - psr_vsc.sdp_header.HB3 = 0xb; > >> + psr_vsc.sdp_header.HB3 = 0xc; > >> + if (dev_priv->psr.y_cord_support && > >> + dev_priv->psr.colorimetry_support) { > >> + psr_vsc.sdp_header.HB2 = 0x5; > >> + psr_vsc.sdp_header.HB3 = 0x13; > >> + } else { > >> + psr_vsc.sdp_header.HB2 = 0x4; > >> + psr_vsc.sdp_header.HB3 = 0xe; > >> + } > > That looks bogus. Why do we claim to have colorimetry data but > > then don't fill it out? > HB2 to be set 04 or 05 > 04h = 3D stereo + PSR/PSR2 + Y-coordinate. > 05h = 3D stereo- + PSR/PSR2 + Y-coordinate + Pixel Encoding/Colorimetry > Format > > As of now it's defaulting to 0x4, will correct it. > > > > Also you're not setting the actual y coordinate stuff anywhere, so why > > would we want to indicate that we support it? > > > Bspec says to set CHICKEN_TRANS_EDP(0x420cc) bit 15 if Y coordinate is > supported. > it set in patch 2. This whole part of the spec looks a wee bit inadequate. Hmm. So bit 25 of CHICKEN_TRANS_EDP seems to control whether the hardware will generate part of the VSC SDP or not. But nowhere does it explain which part that is. The sequence doesn't even mention that bit, but it does mention bit 12 which means "This field enables the programmable header for the PSR2 VSC packet.". Not sure what that means. If that means the hardware can generate the HB0-3 bytes, it would be nice to know what exactly it will put there. And because of this I can't be sure why we have to switch to the manual header mode in PSR2. Maybe it means the hardware generates a header that doesn't allow the Y coordinate stuff. Hmm. Can we maybe read out the hardware generated data via the VSC DIP? And then there are these bits 11 and 15 which controls *something* about the Y coordindate stuff. But it doesn't actually explain if the hardware will fill those out or just send/not send based on these bits. If we assume it will fill them out, then yes, I suppose we should make sure the VSC SDP version and size are high enough to include them. Also how does the hardware handle the SU granularity? I see no bits to control that, so I can't see how the hardware could know what it has to do here. Hmm. PSR2_MAN_TRK_CTL seems to say that when using manual tracking, it's going to send chunks of 4 scanlines always, which I suppose will satisfy the worst case for both the X and Y granularity. So I guess the hardware tracking mode would just do the same. Oh, and while reading the eDP spec, I noticed that the AUX frame sync requires GTC, which we totally don't seem to even configure/enable. We do however tell the sink to enable AUX frame sync whenever it supports it. AUX frame sync is mandatory for PSR2 SU AFAICS, and without SU it doesn't seem to be needed. I haven't spotted any patches for GTC, so I'm asuming it's all still a bit broken. > >> intel_psr_write_vsc(intel_dp, &psr_vsc); > >> } > >> > >> diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h > >> index 63b8bd5..3d875c0 100644 > >> --- a/include/drm/drm_dp_helper.h > >> +++ b/include/drm/drm_dp_helper.h > >> @@ -194,7 +194,7 @@ > >> # define DP_PSR_SETUP_TIME_0 (6 << 1) > >> # define DP_PSR_SETUP_TIME_MASK (7 << 1) > >> # define DP_PSR_SETUP_TIME_SHIFT 1 > >> - > >> +# define DP_PSR_Y_COORDINATE (1 << 4) > >> /* > >> * 0x80-0x8f describe downstream port capabilities, but there are two layouts > >> * based on whether DP_DETAILED_CAP_INFO_AVAILABLE was set. If it was not, > >> @@ -640,6 +640,9 @@ struct edp_sdp_header { > >> #define EDP_SDP_HEADER_REVISION_MASK 0x1F > >> #define EDP_SDP_HEADER_VALID_PAYLOAD_BYTES 0x1F > >> > >> +#define DPRX_FEATURE_ENUMERATION_LIST 0x02210 > >> +#define VSC_SDP_EXT_FOR_COLORIMETRY_SUPPORTED (1 << 3) > >> + > >> struct edp_vsc_psr { > >> struct edp_sdp_header sdp_header; > >> u8 DB0; /* Stereo Interface */ > >> -- > >> 2.7.4 > >> > >> _______________________________________________ > >> Intel-gfx mailing list > >> Intel-gfx@lists.freedesktop.org > >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Friday 12 August 2016 12:02 PM, Ville Syrjälä wrote: > On Fri, Aug 12, 2016 at 10:48:12AM +0530, vathsala nagaraju wrote: >> On Thursday 11 August 2016 01:30 PM, Ville Syrjälä wrote: >>> On Thu, Aug 11, 2016 at 01:07:50PM +0530, vathsala nagaraju wrote: >>>> Adds Y-co-ordinate support to skl_psr_setup_vsc as >>>> per edp 1.4 spec,table 6-11:VSC SDP HEADER >>>> Extension for psr2 support. >>>> >>>> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> >>>> Signed-off-by: vathsala nagaraju <vathsala.nagaraju@intel.com> >>>> --- >>>> drivers/gpu/drm/i915/i915_drv.h | 2 ++ >>>> drivers/gpu/drm/i915/intel_dp.c | 22 ++++++++++++++++++++++ >>>> drivers/gpu/drm/i915/intel_psr.c | 13 ++++++++++++- >>>> include/drm/drm_dp_helper.h | 5 ++++- >>>> 4 files changed, 40 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h >>>> index 7f2754a..79ce64f 100644 >>>> --- a/drivers/gpu/drm/i915/i915_drv.h >>>> +++ b/drivers/gpu/drm/i915/i915_drv.h >>>> @@ -1022,6 +1022,8 @@ struct i915_psr { >>>> bool psr2_support; >>>> bool aux_frame_sync; >>>> bool link_standby; >>>> + bool y_cord_support; >>>> + bool colorimetry_support; >>>> }; >>>> >>>> enum intel_pch { >>>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c >>>> index 364db90..19e9ecf 100644 >>>> --- a/drivers/gpu/drm/i915/intel_dp.c >>>> +++ b/drivers/gpu/drm/i915/intel_dp.c >>>> @@ -3439,6 +3439,28 @@ intel_edp_init_dpcd(struct intel_dp *intel_dp) >>>> dev_priv->psr.psr2_support = dev_priv->psr.aux_frame_sync; >>>> DRM_DEBUG_KMS("PSR2 %s on sink", >>>> dev_priv->psr.psr2_support ? "supported" : "not supported"); >>>> + >>>> + if (dev_priv->psr.psr2_support) { >>>> + uint8_t psr_caps, dprx; >>>> + >>>> + /*check if panel supports Y-Cordinate*/ >>>> + drm_dp_dpcd_readb(&intel_dp->aux, >>>> + DP_PSR_CAPS, >>>> + &psr_caps); >>> intel_dp->edp_dpcd[1] >>> >>> We should probably add something resembling dp_link_status() for each >>> DPCD chunk we cache, to make it less confusing to use them. >>> >>>> + if (psr_caps & DP_PSR_Y_COORDINATE) >>>> + dev_priv->psr.y_cord_support = true; >>>> + else >>>> + dev_priv->psr.y_cord_support = false; >>>> + /* check for COLORIMETRY SUPPORT */ >>>> + drm_dp_dpcd_readb(&intel_dp->aux, >>>> + DPRX_FEATURE_ENUMERATION_LIST, >>>> + &dprx); >>>> + if (dprx & VSC_SDP_EXT_FOR_COLORIMETRY_SUPPORTED) >>>> + dev_priv->psr.colorimetry_support = true; >>>> + else >>>> + dev_priv->psr.colorimetry_support = false; >>>> + } >>>> + >>>> } >>>> >>>> /* Read the eDP Display control capabilities registers */ >>>> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c >>>> index 59a21c9..76a630b 100644 >>>> --- a/drivers/gpu/drm/i915/intel_psr.c >>>> +++ b/drivers/gpu/drm/i915/intel_psr.c >>>> @@ -122,13 +122,24 @@ static void vlv_psr_setup_vsc(struct intel_dp *intel_dp) >>>> static void skl_psr_setup_su_vsc(struct intel_dp *intel_dp) >>>> { >>>> struct edp_vsc_psr psr_vsc; >>>> + struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); >>>> + struct drm_device *dev = intel_dig_port->base.base.dev; >>>> + struct drm_i915_private *dev_priv = to_i915(dev); >>>> >>>> /* Prepare VSC Header for SU as per EDP 1.4 spec, Table 6.11 */ >>>> memset(&psr_vsc, 0, sizeof(psr_vsc)); >>>> psr_vsc.sdp_header.HB0 = 0; >>>> psr_vsc.sdp_header.HB1 = 0x7; >>>> psr_vsc.sdp_header.HB2 = 0x3; >>>> - psr_vsc.sdp_header.HB3 = 0xb; >>>> + psr_vsc.sdp_header.HB3 = 0xc; >>>> + if (dev_priv->psr.y_cord_support && >>>> + dev_priv->psr.colorimetry_support) { >>>> + psr_vsc.sdp_header.HB2 = 0x5; >>>> + psr_vsc.sdp_header.HB3 = 0x13; >>>> + } else { >>>> + psr_vsc.sdp_header.HB2 = 0x4; >>>> + psr_vsc.sdp_header.HB3 = 0xe; >>>> + } >>> That looks bogus. Why do we claim to have colorimetry data but >>> then don't fill it out? >> HB2 to be set 04 or 05 >> 04h = 3D stereo + PSR/PSR2 + Y-coordinate. >> 05h = 3D stereo- + PSR/PSR2 + Y-coordinate + Pixel Encoding/Colorimetry >> Format >> >> As of now it's defaulting to 0x4, will correct it. >>> Also you're not setting the actual y coordinate stuff anywhere, so why >>> would we want to indicate that we support it? >>> >> Bspec says to set CHICKEN_TRANS_EDP(0x420cc) bit 15 if Y coordinate is >> supported. >> it set in patch 2. > This whole part of the spec looks a wee bit inadequate. > > Hmm. So bit 25 of CHICKEN_TRANS_EDP seems to control whether the > hardware will generate part of the VSC SDP or not. But nowhere does it > explain which part that is. The sequence doesn't even mention that bit, > but it does mention bit 12 which means "This field enables the > programmable header for the PSR2 VSC packet.". Not sure what that means. This means bit 12 must be enabled to program hb0 to hb3 from software > If that means the hardware can generate the HB0-3 bytes, it would be > nice to know what exactly it will put there. And because of this I can't > be sure why we have to switch to the manual header mode in PSR2. Maybe > it means the hardware generates a header that doesn't allow the Y > coordinate stuff. Hmm. Can we maybe read out the hardware generated data > via the VSC DIP? hardware folks informed that it's not possible to read the data. Only way is through connecting dp analyser. > > And then there are these bits 11 and 15 which controls *something* about > the Y coordindate stuff. But it doesn't actually explain if the hardware > will fill those out or just send/not send based on these bits. If we > assume it will fill them out, then yes, I suppose we should make sure > the VSC SDP version and size are high enough to include them. HW will send based on enable and disable setting of this bit. bit 15 bit 11 Description 0 0 Y-coordinate (DB12-DB13) and Y-valid (DB1 bit6) are disabled. 0 1 Not a valid setting. 1 0 Y-coordinate (VSC bytes DB12-DB13) is sent and Y-valid (DB1 bit6) is included. 1 1 Y-coordinate (VSC bytes DB12-DB13) is sent and Y-valid (DB1 bit6) is not included. > > Also how does the hardware handle the SU granularity? I see no bits to > control that, so I can't see how the hardware could know what it has to > do here. Hmm. PSR2_MAN_TRK_CTL seems to say that when using manual > tracking, it's going to send chunks of 4 scanlines always, which I > suppose will satisfy the worst case for both the X and Y granularity. > So I guess the hardware tracking mode would just do the same. HW supports SU granularity of 4 lines. > Oh, and while reading the eDP spec, I noticed that the AUX frame sync > requires GTC, which we totally don't seem to even configure/enable. > We do however tell the sink to enable AUX frame sync whenever it > supports it. AUX frame sync is mandatory for PSR2 SU AFAICS, and > without SU it doesn't seem to be needed. I haven't spotted any patches > for GTC, so I'm asuming it's all still a bit broken. > >>>> intel_psr_write_vsc(intel_dp, &psr_vsc); >>>> } >>>> >>>> diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h >>>> index 63b8bd5..3d875c0 100644 >>>> --- a/include/drm/drm_dp_helper.h >>>> +++ b/include/drm/drm_dp_helper.h >>>> @@ -194,7 +194,7 @@ >>>> # define DP_PSR_SETUP_TIME_0 (6 << 1) >>>> # define DP_PSR_SETUP_TIME_MASK (7 << 1) >>>> # define DP_PSR_SETUP_TIME_SHIFT 1 >>>> - >>>> +# define DP_PSR_Y_COORDINATE (1 << 4) >>>> /* >>>> * 0x80-0x8f describe downstream port capabilities, but there are two layouts >>>> * based on whether DP_DETAILED_CAP_INFO_AVAILABLE was set. If it was not, >>>> @@ -640,6 +640,9 @@ struct edp_sdp_header { >>>> #define EDP_SDP_HEADER_REVISION_MASK 0x1F >>>> #define EDP_SDP_HEADER_VALID_PAYLOAD_BYTES 0x1F >>>> >>>> +#define DPRX_FEATURE_ENUMERATION_LIST 0x02210 >>>> +#define VSC_SDP_EXT_FOR_COLORIMETRY_SUPPORTED (1 << 3) >>>> + >>>> struct edp_vsc_psr { >>>> struct edp_sdp_header sdp_header; >>>> u8 DB0; /* Stereo Interface */ >>>> -- >>>> 2.7.4 >>>> >>>> _______________________________________________ >>>> Intel-gfx mailing list >>>> Intel-gfx@lists.freedesktop.org >>>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 7f2754a..79ce64f 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1022,6 +1022,8 @@ struct i915_psr { bool psr2_support; bool aux_frame_sync; bool link_standby; + bool y_cord_support; + bool colorimetry_support; }; enum intel_pch { diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 364db90..19e9ecf 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -3439,6 +3439,28 @@ intel_edp_init_dpcd(struct intel_dp *intel_dp) dev_priv->psr.psr2_support = dev_priv->psr.aux_frame_sync; DRM_DEBUG_KMS("PSR2 %s on sink", dev_priv->psr.psr2_support ? "supported" : "not supported"); + + if (dev_priv->psr.psr2_support) { + uint8_t psr_caps, dprx; + + /*check if panel supports Y-Cordinate*/ + drm_dp_dpcd_readb(&intel_dp->aux, + DP_PSR_CAPS, + &psr_caps); + if (psr_caps & DP_PSR_Y_COORDINATE) + dev_priv->psr.y_cord_support = true; + else + dev_priv->psr.y_cord_support = false; + /* check for COLORIMETRY SUPPORT */ + drm_dp_dpcd_readb(&intel_dp->aux, + DPRX_FEATURE_ENUMERATION_LIST, + &dprx); + if (dprx & VSC_SDP_EXT_FOR_COLORIMETRY_SUPPORTED) + dev_priv->psr.colorimetry_support = true; + else + dev_priv->psr.colorimetry_support = false; + } + } /* Read the eDP Display control capabilities registers */ diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c index 59a21c9..76a630b 100644 --- a/drivers/gpu/drm/i915/intel_psr.c +++ b/drivers/gpu/drm/i915/intel_psr.c @@ -122,13 +122,24 @@ static void vlv_psr_setup_vsc(struct intel_dp *intel_dp) static void skl_psr_setup_su_vsc(struct intel_dp *intel_dp) { struct edp_vsc_psr psr_vsc; + struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); + struct drm_device *dev = intel_dig_port->base.base.dev; + struct drm_i915_private *dev_priv = to_i915(dev); /* Prepare VSC Header for SU as per EDP 1.4 spec, Table 6.11 */ memset(&psr_vsc, 0, sizeof(psr_vsc)); psr_vsc.sdp_header.HB0 = 0; psr_vsc.sdp_header.HB1 = 0x7; psr_vsc.sdp_header.HB2 = 0x3; - psr_vsc.sdp_header.HB3 = 0xb; + psr_vsc.sdp_header.HB3 = 0xc; + if (dev_priv->psr.y_cord_support && + dev_priv->psr.colorimetry_support) { + psr_vsc.sdp_header.HB2 = 0x5; + psr_vsc.sdp_header.HB3 = 0x13; + } else { + psr_vsc.sdp_header.HB2 = 0x4; + psr_vsc.sdp_header.HB3 = 0xe; + } intel_psr_write_vsc(intel_dp, &psr_vsc); } diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h index 63b8bd5..3d875c0 100644 --- a/include/drm/drm_dp_helper.h +++ b/include/drm/drm_dp_helper.h @@ -194,7 +194,7 @@ # define DP_PSR_SETUP_TIME_0 (6 << 1) # define DP_PSR_SETUP_TIME_MASK (7 << 1) # define DP_PSR_SETUP_TIME_SHIFT 1 - +# define DP_PSR_Y_COORDINATE (1 << 4) /* * 0x80-0x8f describe downstream port capabilities, but there are two layouts * based on whether DP_DETAILED_CAP_INFO_AVAILABLE was set. If it was not, @@ -640,6 +640,9 @@ struct edp_sdp_header { #define EDP_SDP_HEADER_REVISION_MASK 0x1F #define EDP_SDP_HEADER_VALID_PAYLOAD_BYTES 0x1F +#define DPRX_FEATURE_ENUMERATION_LIST 0x02210 +#define VSC_SDP_EXT_FOR_COLORIMETRY_SUPPORTED (1 << 3) + struct edp_vsc_psr { struct edp_sdp_header sdp_header; u8 DB0; /* Stereo Interface */
Adds Y-co-ordinate support to skl_psr_setup_vsc as per edp 1.4 spec,table 6-11:VSC SDP HEADER Extension for psr2 support. Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> Signed-off-by: vathsala nagaraju <vathsala.nagaraju@intel.com> --- drivers/gpu/drm/i915/i915_drv.h | 2 ++ drivers/gpu/drm/i915/intel_dp.c | 22 ++++++++++++++++++++++ drivers/gpu/drm/i915/intel_psr.c | 13 ++++++++++++- include/drm/drm_dp_helper.h | 5 ++++- 4 files changed, 40 insertions(+), 2 deletions(-)