| Message ID | 20180322214848.28022-4-jose.souza@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Mar 22, 2018 at 02:48:40PM -0700, José Roberto de Souza wrote: > Move to only one place the sink requirements that the actual driver > needs to enable PSR2. > > Also intel_psr2_config_valid() is called every time the crtc config > is computed, wasting some time every time it was checking for > Y coordinate requirement. > > This allow us to nuke y_cord_support and some of VSC setup code that > was handling a scenario that would never happen(PSR2 without Y > coordinate). > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > --- > drivers/gpu/drm/i915/i915_drv.h | 1 - > drivers/gpu/drm/i915/intel_psr.c | 46 +++++++++++++++++----------------------- > 2 files changed, 19 insertions(+), 28 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 7fe00509e51a..cce32686fd75 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -603,7 +603,6 @@ struct i915_psr { > unsigned busy_frontbuffer_bits; > bool psr2_support; > bool link_standby; > - bool y_cord_support; > bool colorimetry_support; > bool alpm; > bool has_hw_tracking; > diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c > index d46320a451d9..23f38ab10636 100644 > --- a/drivers/gpu/drm/i915/intel_psr.c > +++ b/drivers/gpu/drm/i915/intel_psr.c > @@ -93,7 +93,7 @@ static void psr_aux_io_power_put(struct intel_dp *intel_dp) > intel_display_power_put(dev_priv, psr_aux_domain(intel_dp)); > } > > -static bool intel_dp_get_y_cord_status(struct intel_dp *intel_dp) > +static bool intel_dp_get_y_coord_required(struct intel_dp *intel_dp) nip: I don't agree that required is a good name for this function call so maybe we should just leave status and minimize the change. > { > uint8_t psr_caps = 0; > > @@ -130,22 +130,29 @@ void intel_psr_init_dpcd(struct intel_dp *intel_dp) > drm_dp_dpcd_read(&intel_dp->aux, DP_PSR_SUPPORT, intel_dp->psr_dpcd, > sizeof(intel_dp->psr_dpcd)); > > - if (intel_dp->psr_dpcd[0] & DP_PSR_IS_SUPPORTED) { > + if (intel_dp->psr_dpcd[0]) { hm? why? > dev_priv->psr.sink_support = true; > DRM_DEBUG_KMS("Detected EDP PSR Panel.\n"); > } > > if (INTEL_GEN(dev_priv) >= 9 && > - (intel_dp->psr_dpcd[0] & DP_PSR2_IS_SUPPORTED)) { > - > - dev_priv->psr.sink_support = true; > - dev_priv->psr.psr2_support = true; > + (intel_dp->psr_dpcd[0] == DP_PSR2_WITH_Y_COORD_IS_SUPPORTED)) { > + /* > + * All panels that supports PSR version 03h (PSR2 + > + * Y-coordinate) can handle Y-coordinates in VSC but we are > + * only sure that it is going to be used when required by the > + * panel. This way panel is capable to do selective update > + * without a aux frame sync. > + * > + * To support PSR version 02h and PSR version 03h without > + * Y-coordinate requirement panels we would need to enable > + * GTC first. > + */ > + dev_priv->psr.psr2_support = intel_dp_get_y_coord_required(intel_dp); oh ok ok... now I saw why you changed to "required" But my question now is. Do we really need to check this bit? I believe with your change to check version 03h we don't need to check this requirement anymore. This looks like in the past we used that to workaround a way to identify if the panel was really y-coord... but now with 03h version we don't need to check if panel is requiring the y-coordination from the source, because we know what source is capable of. > DRM_DEBUG_KMS("PSR2 %s on sink", > dev_priv->psr.psr2_support ? "supported" : "not supported"); > > if (dev_priv->psr.psr2_support) { > - dev_priv->psr.y_cord_support = > - intel_dp_get_y_cord_status(intel_dp); > dev_priv->psr.colorimetry_support = > intel_dp_get_colorimetry_status(intel_dp); > dev_priv->psr.alpm = > @@ -191,16 +198,12 @@ static void hsw_psr_setup_vsc(struct intel_dp *intel_dp, > memset(&psr_vsc, 0, sizeof(psr_vsc)); > psr_vsc.sdp_header.HB0 = 0; > psr_vsc.sdp_header.HB1 = 0x7; > - if (dev_priv->psr.colorimetry_support && > - dev_priv->psr.y_cord_support) { > + if (dev_priv->psr.colorimetry_support) { > psr_vsc.sdp_header.HB2 = 0x5; > psr_vsc.sdp_header.HB3 = 0x13; > - } else if (dev_priv->psr.y_cord_support) { > + } else { > psr_vsc.sdp_header.HB2 = 0x4; > psr_vsc.sdp_header.HB3 = 0xe; > - } else { > - psr_vsc.sdp_header.HB2 = 0x3; > - psr_vsc.sdp_header.HB3 = 0xc; > } > } else { > /* Prepare VSC packet as per EDP 1.3 spec, Table 3.10 */ > @@ -458,15 +461,6 @@ static bool intel_psr2_config_valid(struct intel_dp *intel_dp, > return false; > } > > - /* > - * FIXME:enable psr2 only for y-cordinate psr2 panels > - * After gtc implementation , remove this restriction. > - */ > - if (!dev_priv->psr.y_cord_support) { > - DRM_DEBUG_KMS("PSR2 not enabled, panel does not support Y coordinate\n"); > - return false; > - } > - > return true; > } > > @@ -566,7 +560,6 @@ static void hsw_psr_enable_source(struct intel_dp *intel_dp, > struct drm_device *dev = dig_port->base.base.dev; > struct drm_i915_private *dev_priv = to_i915(dev); > enum transcoder cpu_transcoder = crtc_state->cpu_transcoder; > - u32 chicken; > > psr_aux_io_power_get(intel_dp); > > @@ -577,9 +570,8 @@ static void hsw_psr_enable_source(struct intel_dp *intel_dp, > hsw_psr_setup_aux(intel_dp); > > if (dev_priv->psr.psr2_support) { > - chicken = PSR2_VSC_ENABLE_PROG_HEADER; > - if (dev_priv->psr.y_cord_support) > - chicken |= PSR2_ADD_VERTICAL_LINE_COUNT; > + u32 chicken = PSR2_VSC_ENABLE_PROG_HEADER > + | PSR2_ADD_VERTICAL_LINE_COUNT; > I915_WRITE(CHICKEN_TRANS(cpu_transcoder), chicken); > > I915_WRITE(EDP_PSR_DEBUG, > -- > 2.16.2 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Thu, 2018-03-22 at 16:09 -0700, Rodrigo Vivi wrote: > On Thu, Mar 22, 2018 at 02:48:40PM -0700, José Roberto de Souza > wrote: > > Move to only one place the sink requirements that the actual driver > > needs to enable PSR2. > > > > Also intel_psr2_config_valid() is called every time the crtc config > > is computed, wasting some time every time it was checking for > > Y coordinate requirement. > > > > This allow us to nuke y_cord_support and some of VSC setup code > > that > > was handling a scenario that would never happen(PSR2 without Y > > coordinate). > > > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > > --- > > drivers/gpu/drm/i915/i915_drv.h | 1 - > > drivers/gpu/drm/i915/intel_psr.c | 46 +++++++++++++++++----------- > > ------------ > > 2 files changed, 19 insertions(+), 28 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h > > b/drivers/gpu/drm/i915/i915_drv.h > > index 7fe00509e51a..cce32686fd75 100644 > > --- a/drivers/gpu/drm/i915/i915_drv.h > > +++ b/drivers/gpu/drm/i915/i915_drv.h > > @@ -603,7 +603,6 @@ struct i915_psr { > > unsigned busy_frontbuffer_bits; > > bool psr2_support; > > bool link_standby; > > - bool y_cord_support; > > bool colorimetry_support; > > bool alpm; > > bool has_hw_tracking; > > diff --git a/drivers/gpu/drm/i915/intel_psr.c > > b/drivers/gpu/drm/i915/intel_psr.c > > index d46320a451d9..23f38ab10636 100644 > > --- a/drivers/gpu/drm/i915/intel_psr.c > > +++ b/drivers/gpu/drm/i915/intel_psr.c > > @@ -93,7 +93,7 @@ static void psr_aux_io_power_put(struct intel_dp > > *intel_dp) > > intel_display_power_put(dev_priv, > > psr_aux_domain(intel_dp)); > > } > > > > -static bool intel_dp_get_y_cord_status(struct intel_dp *intel_dp) > > +static bool intel_dp_get_y_coord_required(struct intel_dp > > *intel_dp) > > nip: I don't agree that required is a good name for this function > call > so maybe we should just leave status and minimize the change. This is the name in the eDP spec: Y-coordinate Requirement of Sink Device on PSR2 Selective Update > > > { > > uint8_t psr_caps = 0; > > > > @@ -130,22 +130,29 @@ void intel_psr_init_dpcd(struct intel_dp > > *intel_dp) > > drm_dp_dpcd_read(&intel_dp->aux, DP_PSR_SUPPORT, intel_dp- > > >psr_dpcd, > > sizeof(intel_dp->psr_dpcd)); > > > > - if (intel_dp->psr_dpcd[0] & DP_PSR_IS_SUPPORTED) { > > + if (intel_dp->psr_dpcd[0]) { > > hm? why? A pannel that is PSR version 2, will not have any PSR enabled as 'intel_dp->psr_dpcd[0] & DP_PSR_IS_SUPPORTED' and 'intel_dp- >psr_dpcd[0] == DP_PSR2_WITH_Y_COORD_IS_SUPPORTED' will be false. > > > dev_priv->psr.sink_support = true; > > DRM_DEBUG_KMS("Detected EDP PSR Panel.\n"); > > } > > > > if (INTEL_GEN(dev_priv) >= 9 && > > - (intel_dp->psr_dpcd[0] & DP_PSR2_IS_SUPPORTED)) { > > - > > - dev_priv->psr.sink_support = true; > > - dev_priv->psr.psr2_support = true; > > + (intel_dp->psr_dpcd[0] == > > DP_PSR2_WITH_Y_COORD_IS_SUPPORTED)) { > > + /* > > + * All panels that supports PSR version 03h (PSR2 > > + > > + * Y-coordinate) can handle Y-coordinates in VSC > > but we are > > + * only sure that it is going to be used when > > required by the > > + * panel. This way panel is capable to do > > selective update > > + * without a aux frame sync. > > + * > > + * To support PSR version 02h and PSR version 03h > > without > > + * Y-coordinate requirement panels we would need > > to enable > > + * GTC first. > > + */ > > + dev_priv->psr.psr2_support = > > intel_dp_get_y_coord_required(intel_dp); > > oh ok ok... now I saw why you changed to "required" > > But my question now is. Do we really need to check this bit? > > I believe with your change to check version 03h we don't need to > check > this requirement anymore. > This looks like in the past we used that to workaround a way to > identify > if the panel was really y-coord... but now with 03h version we don't > need > to check if panel is requiring the y-coordination from the source, > because > we know what source is capable of. We know that is capable of but we don't know if is going to be used by the pannel, when pannels require the y-coordinate we are 100% sure that it is going to use. > > > DRM_DEBUG_KMS("PSR2 %s on sink", > > dev_priv->psr.psr2_support ? > > "supported" : "not supported"); > > > > if (dev_priv->psr.psr2_support) { > > - dev_priv->psr.y_cord_support = > > - intel_dp_get_y_cord_status(intel_d > > p); > > dev_priv->psr.colorimetry_support = > > intel_dp_get_colorimetry_status(in > > tel_dp); > > dev_priv->psr.alpm = > > @@ -191,16 +198,12 @@ static void hsw_psr_setup_vsc(struct intel_dp > > *intel_dp, > > memset(&psr_vsc, 0, sizeof(psr_vsc)); > > psr_vsc.sdp_header.HB0 = 0; > > psr_vsc.sdp_header.HB1 = 0x7; > > - if (dev_priv->psr.colorimetry_support && > > - dev_priv->psr.y_cord_support) { > > + if (dev_priv->psr.colorimetry_support) { > > psr_vsc.sdp_header.HB2 = 0x5; > > psr_vsc.sdp_header.HB3 = 0x13; > > - } else if (dev_priv->psr.y_cord_support) { > > + } else { > > psr_vsc.sdp_header.HB2 = 0x4; > > psr_vsc.sdp_header.HB3 = 0xe; > > - } else { > > - psr_vsc.sdp_header.HB2 = 0x3; > > - psr_vsc.sdp_header.HB3 = 0xc; > > } > > } else { > > /* Prepare VSC packet as per EDP 1.3 spec, Table > > 3.10 */ > > @@ -458,15 +461,6 @@ static bool intel_psr2_config_valid(struct > > intel_dp *intel_dp, > > return false; > > } > > > > - /* > > - * FIXME:enable psr2 only for y-cordinate psr2 panels > > - * After gtc implementation , remove this restriction. > > - */ > > - if (!dev_priv->psr.y_cord_support) { > > - DRM_DEBUG_KMS("PSR2 not enabled, panel does not > > support Y coordinate\n"); > > - return false; > > - } > > - > > return true; > > } > > > > @@ -566,7 +560,6 @@ static void hsw_psr_enable_source(struct > > intel_dp *intel_dp, > > struct drm_device *dev = dig_port->base.base.dev; > > struct drm_i915_private *dev_priv = to_i915(dev); > > enum transcoder cpu_transcoder = crtc_state- > > >cpu_transcoder; > > - u32 chicken; > > > > psr_aux_io_power_get(intel_dp); > > > > @@ -577,9 +570,8 @@ static void hsw_psr_enable_source(struct > > intel_dp *intel_dp, > > hsw_psr_setup_aux(intel_dp); > > > > if (dev_priv->psr.psr2_support) { > > - chicken = PSR2_VSC_ENABLE_PROG_HEADER; > > - if (dev_priv->psr.y_cord_support) > > - chicken |= PSR2_ADD_VERTICAL_LINE_COUNT; > > + u32 chicken = PSR2_VSC_ENABLE_PROG_HEADER > > + | PSR2_ADD_VERTICAL_LINE_COUNT; > > I915_WRITE(CHICKEN_TRANS(cpu_transcoder), > > chicken); > > > > I915_WRITE(EDP_PSR_DEBUG, > > -- > > 2.16.2 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Thu, 2018-03-22 at 14:48 -0700, José Roberto de Souza wrote: > Move to only one place the sink requirements that the actual driver > needs to enable PSR2. > > Also intel_psr2_config_valid() is called every time the crtc config > is computed, wasting some time every time it was checking for > Y coordinate requirement. > > This allow us to nuke y_cord_support and some of VSC setup code that > was handling a scenario that would never happen(PSR2 without Y > coordinate). > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > --- > drivers/gpu/drm/i915/i915_drv.h | 1 - > drivers/gpu/drm/i915/intel_psr.c | 46 +++++++++++++++++----------------------- > 2 files changed, 19 insertions(+), 28 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 7fe00509e51a..cce32686fd75 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -603,7 +603,6 @@ struct i915_psr { > unsigned busy_frontbuffer_bits; > bool psr2_support; > bool link_standby; > - bool y_cord_support; > bool colorimetry_support; > bool alpm; > bool has_hw_tracking; > diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c > index d46320a451d9..23f38ab10636 100644 > --- a/drivers/gpu/drm/i915/intel_psr.c > +++ b/drivers/gpu/drm/i915/intel_psr.c > @@ -93,7 +93,7 @@ static void psr_aux_io_power_put(struct intel_dp *intel_dp) > intel_display_power_put(dev_priv, psr_aux_domain(intel_dp)); > } > > -static bool intel_dp_get_y_cord_status(struct intel_dp *intel_dp) > +static bool intel_dp_get_y_coord_required(struct intel_dp *intel_dp) > { > uint8_t psr_caps = 0; > > @@ -130,22 +130,29 @@ void intel_psr_init_dpcd(struct intel_dp *intel_dp) > drm_dp_dpcd_read(&intel_dp->aux, DP_PSR_SUPPORT, intel_dp->psr_dpcd, > sizeof(intel_dp->psr_dpcd)); > > - if (intel_dp->psr_dpcd[0] & DP_PSR_IS_SUPPORTED) { > + if (intel_dp->psr_dpcd[0]) { > dev_priv->psr.sink_support = true; > DRM_DEBUG_KMS("Detected EDP PSR Panel.\n"); > } > > if (INTEL_GEN(dev_priv) >= 9 && > - (intel_dp->psr_dpcd[0] & DP_PSR2_IS_SUPPORTED)) { > - > - dev_priv->psr.sink_support = true; > - dev_priv->psr.psr2_support = true; > + (intel_dp->psr_dpcd[0] == DP_PSR2_WITH_Y_COORD_IS_SUPPORTED)) { > + /* > + * All panels that supports PSR version 03h (PSR2 + > + * Y-coordinate) can handle Y-coordinates in VSC but we are > + * only sure that it is going to be used when required by the > + * panel. This way panel is capable to do selective update > + * without a aux frame sync. Can you cite this from the spec please? just the "panel is capable to do selective update without a aux frame sync" part. Or you could just remove that line so that we can get this patch reviewed and merged. > + * > + * To support PSR version 02h and PSR version 03h without > + * Y-coordinate requirement panels we would need to enable > + * GTC first. > + */ > + dev_priv->psr.psr2_support = intel_dp_get_y_coord_required(intel_dp); > DRM_DEBUG_KMS("PSR2 %s on sink", > dev_priv->psr.psr2_support ? "supported" : "not supported"); > > if (dev_priv->psr.psr2_support) { > - dev_priv->psr.y_cord_support = > - intel_dp_get_y_cord_status(intel_dp); > dev_priv->psr.colorimetry_support = > intel_dp_get_colorimetry_status(intel_dp); > dev_priv->psr.alpm = > @@ -191,16 +198,12 @@ static void hsw_psr_setup_vsc(struct intel_dp *intel_dp, > memset(&psr_vsc, 0, sizeof(psr_vsc)); > psr_vsc.sdp_header.HB0 = 0; > psr_vsc.sdp_header.HB1 = 0x7; > - if (dev_priv->psr.colorimetry_support && > - dev_priv->psr.y_cord_support) { > + if (dev_priv->psr.colorimetry_support) { > psr_vsc.sdp_header.HB2 = 0x5; > psr_vsc.sdp_header.HB3 = 0x13; > - } else if (dev_priv->psr.y_cord_support) { > + } else { > psr_vsc.sdp_header.HB2 = 0x4; > psr_vsc.sdp_header.HB3 = 0xe; > - } else { > - psr_vsc.sdp_header.HB2 = 0x3; > - psr_vsc.sdp_header.HB3 = 0xc; > } > } else { > /* Prepare VSC packet as per EDP 1.3 spec, Table 3.10 */ > @@ -458,15 +461,6 @@ static bool intel_psr2_config_valid(struct intel_dp *intel_dp, > return false; > } > > - /* > - * FIXME:enable psr2 only for y-cordinate psr2 panels > - * After gtc implementation , remove this restriction. > - */ > - if (!dev_priv->psr.y_cord_support) { > - DRM_DEBUG_KMS("PSR2 not enabled, panel does not support Y coordinate\n"); > - return false; > - } > - > return true; > } > > @@ -566,7 +560,6 @@ static void hsw_psr_enable_source(struct intel_dp *intel_dp, > struct drm_device *dev = dig_port->base.base.dev; > struct drm_i915_private *dev_priv = to_i915(dev); > enum transcoder cpu_transcoder = crtc_state->cpu_transcoder; > - u32 chicken; > > psr_aux_io_power_get(intel_dp); > > @@ -577,9 +570,8 @@ static void hsw_psr_enable_source(struct intel_dp *intel_dp, > hsw_psr_setup_aux(intel_dp); > > if (dev_priv->psr.psr2_support) { > - chicken = PSR2_VSC_ENABLE_PROG_HEADER; > - if (dev_priv->psr.y_cord_support) > - chicken |= PSR2_ADD_VERTICAL_LINE_COUNT; > + u32 chicken = PSR2_VSC_ENABLE_PROG_HEADER > + | PSR2_ADD_VERTICAL_LINE_COUNT; > I915_WRITE(CHICKEN_TRANS(cpu_transcoder), chicken); > > I915_WRITE(EDP_PSR_DEBUG,
On Fri, 2018-03-23 at 15:59 -0700, Pandiyan, Dhinakaran wrote: > > > On Thu, 2018-03-22 at 14:48 -0700, José Roberto de Souza wrote: > > Move to only one place the sink requirements that the actual driver > > needs to enable PSR2. > > > > Also intel_psr2_config_valid() is called every time the crtc config > > is computed, wasting some time every time it was checking for > > Y coordinate requirement. > > > > This allow us to nuke y_cord_support and some of VSC setup code > > that > > was handling a scenario that would never happen(PSR2 without Y > > coordinate). > > > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > > --- > > drivers/gpu/drm/i915/i915_drv.h | 1 - > > drivers/gpu/drm/i915/intel_psr.c | 46 +++++++++++++++++----------- > > ------------ > > 2 files changed, 19 insertions(+), 28 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h > > b/drivers/gpu/drm/i915/i915_drv.h > > index 7fe00509e51a..cce32686fd75 100644 > > --- a/drivers/gpu/drm/i915/i915_drv.h > > +++ b/drivers/gpu/drm/i915/i915_drv.h > > @@ -603,7 +603,6 @@ struct i915_psr { > > unsigned busy_frontbuffer_bits; > > bool psr2_support; > > bool link_standby; > > - bool y_cord_support; > > bool colorimetry_support; > > bool alpm; > > bool has_hw_tracking; > > diff --git a/drivers/gpu/drm/i915/intel_psr.c > > b/drivers/gpu/drm/i915/intel_psr.c > > index d46320a451d9..23f38ab10636 100644 > > --- a/drivers/gpu/drm/i915/intel_psr.c > > +++ b/drivers/gpu/drm/i915/intel_psr.c > > @@ -93,7 +93,7 @@ static void psr_aux_io_power_put(struct intel_dp > > *intel_dp) > > intel_display_power_put(dev_priv, > > psr_aux_domain(intel_dp)); > > } > > > > -static bool intel_dp_get_y_cord_status(struct intel_dp *intel_dp) > > +static bool intel_dp_get_y_coord_required(struct intel_dp > > *intel_dp) > > { > > uint8_t psr_caps = 0; > > > > @@ -130,22 +130,29 @@ void intel_psr_init_dpcd(struct intel_dp > > *intel_dp) > > drm_dp_dpcd_read(&intel_dp->aux, DP_PSR_SUPPORT, intel_dp- > > >psr_dpcd, > > sizeof(intel_dp->psr_dpcd)); > > > > - if (intel_dp->psr_dpcd[0] & DP_PSR_IS_SUPPORTED) { > > + if (intel_dp->psr_dpcd[0]) { > > dev_priv->psr.sink_support = true; > > DRM_DEBUG_KMS("Detected EDP PSR Panel.\n"); > > } > > > > if (INTEL_GEN(dev_priv) >= 9 && > > - (intel_dp->psr_dpcd[0] & DP_PSR2_IS_SUPPORTED)) { > > - > > - dev_priv->psr.sink_support = true; > > - dev_priv->psr.psr2_support = true; > > + (intel_dp->psr_dpcd[0] == > > DP_PSR2_WITH_Y_COORD_IS_SUPPORTED)) { > > + /* > > + * All panels that supports PSR version 03h (PSR2 > > + > > + * Y-coordinate) can handle Y-coordinates in VSC > > but we are > > + * only sure that it is going to be used when > > required by the > > + * panel. This way panel is capable to do > > selective update > > + * without a aux frame sync. > > Can you cite this from the spec please? just the "panel is capable to > do > selective update without a aux frame sync" part. Or you could just > remove that line so that we can get this patch reviewed and merged. There is no such statement in spec like this, the closest that it have is: "When the Source device includes the optional Y-coordinate in the SDP for PSR2 Operation, as described in Table 6-12, the Sink device can implement a lower-precision GTC Slave function, as described in Table 7-1." But we know that this works by all the previous tests when enabling PSR2 and I also tested it. > > > > + * > > + * To support PSR version 02h and PSR version 03h > > without > > + * Y-coordinate requirement panels we would need > > to enable > > + * GTC first. > > + */ > > + dev_priv->psr.psr2_support = > > intel_dp_get_y_coord_required(intel_dp); > > DRM_DEBUG_KMS("PSR2 %s on sink", > > dev_priv->psr.psr2_support ? > > "supported" : "not supported"); > > > > if (dev_priv->psr.psr2_support) { > > - dev_priv->psr.y_cord_support = > > - intel_dp_get_y_cord_status(intel_d > > p); > > dev_priv->psr.colorimetry_support = > > intel_dp_get_colorimetry_status(in > > tel_dp); > > dev_priv->psr.alpm = > > @@ -191,16 +198,12 @@ static void hsw_psr_setup_vsc(struct intel_dp > > *intel_dp, > > memset(&psr_vsc, 0, sizeof(psr_vsc)); > > psr_vsc.sdp_header.HB0 = 0; > > psr_vsc.sdp_header.HB1 = 0x7; > > - if (dev_priv->psr.colorimetry_support && > > - dev_priv->psr.y_cord_support) { > > + if (dev_priv->psr.colorimetry_support) { > > psr_vsc.sdp_header.HB2 = 0x5; > > psr_vsc.sdp_header.HB3 = 0x13; > > - } else if (dev_priv->psr.y_cord_support) { > > + } else { > > psr_vsc.sdp_header.HB2 = 0x4; > > psr_vsc.sdp_header.HB3 = 0xe; > > - } else { > > - psr_vsc.sdp_header.HB2 = 0x3; > > - psr_vsc.sdp_header.HB3 = 0xc; > > } > > } else { > > /* Prepare VSC packet as per EDP 1.3 spec, Table > > 3.10 */ > > @@ -458,15 +461,6 @@ static bool intel_psr2_config_valid(struct > > intel_dp *intel_dp, > > return false; > > } > > > > - /* > > - * FIXME:enable psr2 only for y-cordinate psr2 panels > > - * After gtc implementation , remove this restriction. > > - */ > > - if (!dev_priv->psr.y_cord_support) { > > - DRM_DEBUG_KMS("PSR2 not enabled, panel does not > > support Y coordinate\n"); > > - return false; > > - } > > - > > return true; > > } > > > > @@ -566,7 +560,6 @@ static void hsw_psr_enable_source(struct > > intel_dp *intel_dp, > > struct drm_device *dev = dig_port->base.base.dev; > > struct drm_i915_private *dev_priv = to_i915(dev); > > enum transcoder cpu_transcoder = crtc_state- > > >cpu_transcoder; > > - u32 chicken; > > > > psr_aux_io_power_get(intel_dp); > > > > @@ -577,9 +570,8 @@ static void hsw_psr_enable_source(struct > > intel_dp *intel_dp, > > hsw_psr_setup_aux(intel_dp); > > > > if (dev_priv->psr.psr2_support) { > > - chicken = PSR2_VSC_ENABLE_PROG_HEADER; > > - if (dev_priv->psr.y_cord_support) > > - chicken |= PSR2_ADD_VERTICAL_LINE_COUNT; > > + u32 chicken = PSR2_VSC_ENABLE_PROG_HEADER > > + | PSR2_ADD_VERTICAL_LINE_COUNT; > > I915_WRITE(CHICKEN_TRANS(cpu_transcoder), > > chicken); > > > > I915_WRITE(EDP_PSR_DEBUG,
On Fri, 2018-03-23 at 23:51 +0000, Souza, Jose wrote: > On Fri, 2018-03-23 at 15:59 -0700, Pandiyan, Dhinakaran wrote: > > > > > > On Thu, 2018-03-22 at 14:48 -0700, José Roberto de Souza wrote: > > > Move to only one place the sink requirements that the actual driver > > > needs to enable PSR2. > > > > > > Also intel_psr2_config_valid() is called every time the crtc config > > > is computed, wasting some time every time it was checking for > > > Y coordinate requirement. > > > > > > This allow us to nuke y_cord_support and some of VSC setup code > > > that > > > was handling a scenario that would never happen(PSR2 without Y > > > coordinate). > > > > > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > > > --- > > > drivers/gpu/drm/i915/i915_drv.h | 1 - > > > drivers/gpu/drm/i915/intel_psr.c | 46 +++++++++++++++++----------- > > > ------------ > > > 2 files changed, 19 insertions(+), 28 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h > > > b/drivers/gpu/drm/i915/i915_drv.h > > > index 7fe00509e51a..cce32686fd75 100644 > > > --- a/drivers/gpu/drm/i915/i915_drv.h > > > +++ b/drivers/gpu/drm/i915/i915_drv.h > > > @@ -603,7 +603,6 @@ struct i915_psr { > > > unsigned busy_frontbuffer_bits; > > > bool psr2_support; > > > bool link_standby; > > > - bool y_cord_support; > > > bool colorimetry_support; > > > bool alpm; > > > bool has_hw_tracking; > > > diff --git a/drivers/gpu/drm/i915/intel_psr.c > > > b/drivers/gpu/drm/i915/intel_psr.c > > > index d46320a451d9..23f38ab10636 100644 > > > --- a/drivers/gpu/drm/i915/intel_psr.c > > > +++ b/drivers/gpu/drm/i915/intel_psr.c > > > @@ -93,7 +93,7 @@ static void psr_aux_io_power_put(struct intel_dp > > > *intel_dp) > > > intel_display_power_put(dev_priv, > > > psr_aux_domain(intel_dp)); > > > } > > > > > > -static bool intel_dp_get_y_cord_status(struct intel_dp *intel_dp) > > > +static bool intel_dp_get_y_coord_required(struct intel_dp > > > *intel_dp) > > > { > > > uint8_t psr_caps = 0; > > > > > > @@ -130,22 +130,29 @@ void intel_psr_init_dpcd(struct intel_dp > > > *intel_dp) > > > drm_dp_dpcd_read(&intel_dp->aux, DP_PSR_SUPPORT, intel_dp- > > > >psr_dpcd, > > > sizeof(intel_dp->psr_dpcd)); > > > > > > - if (intel_dp->psr_dpcd[0] & DP_PSR_IS_SUPPORTED) { > > > + if (intel_dp->psr_dpcd[0]) { > > > dev_priv->psr.sink_support = true; > > > DRM_DEBUG_KMS("Detected EDP PSR Panel.\n"); > > > } > > > > > > if (INTEL_GEN(dev_priv) >= 9 && > > > - (intel_dp->psr_dpcd[0] & DP_PSR2_IS_SUPPORTED)) { > > > - > > > - dev_priv->psr.sink_support = true; > > > - dev_priv->psr.psr2_support = true; > > > + (intel_dp->psr_dpcd[0] == > > > DP_PSR2_WITH_Y_COORD_IS_SUPPORTED)) { > > > + /* > > > + * All panels that supports PSR version 03h (PSR2 > > > + > > > + * Y-coordinate) can handle Y-coordinates in VSC > > > but we are > > > + * only sure that it is going to be used when > > > required by the > > > + * panel. This way panel is capable to do > > > selective update > > > + * without a aux frame sync. > > > > Can you cite this from the spec please? just the "panel is capable to > > do > > selective update without a aux frame sync" part. Or you could just > > remove that line so that we can get this patch reviewed and merged. > > There is no such statement in spec like this, the closest that it have > is: > "When the Source device includes the optional Y-coordinate in the SDP > for PSR2 Operation, as described in Table 6-12, the Sink device can > implement a lower-precision GTC Slave function, as described in Table > 7-1." > > But we know that this works by all the previous tests when enabling > PSR2 and I also tested it. Please do update the comment to say so. Do you know if these tests for PSR2 selective update are in IGT? Or were these manual tests? Rodrigo/Vathsala, any idea how selective update was and can be tested? The patch as such makes sense to me Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> if you update the comment. > > > > > > > > + * > > > + * To support PSR version 02h and PSR version 03h > > > without > > > + * Y-coordinate requirement panels we would need > > > to enable > > > + * GTC first. > > > + */ > > > + dev_priv->psr.psr2_support = > > > intel_dp_get_y_coord_required(intel_dp); > > > DRM_DEBUG_KMS("PSR2 %s on sink", > > > dev_priv->psr.psr2_support ? > > > "supported" : "not supported"); > > > > > > if (dev_priv->psr.psr2_support) { > > > - dev_priv->psr.y_cord_support = > > > - intel_dp_get_y_cord_status(intel_d > > > p); > > > dev_priv->psr.colorimetry_support = > > > intel_dp_get_colorimetry_status(in > > > tel_dp); > > > dev_priv->psr.alpm = > > > @@ -191,16 +198,12 @@ static void hsw_psr_setup_vsc(struct intel_dp > > > *intel_dp, > > > memset(&psr_vsc, 0, sizeof(psr_vsc)); > > > psr_vsc.sdp_header.HB0 = 0; > > > psr_vsc.sdp_header.HB1 = 0x7; > > > - if (dev_priv->psr.colorimetry_support && > > > - dev_priv->psr.y_cord_support) { > > > + if (dev_priv->psr.colorimetry_support) { > > > psr_vsc.sdp_header.HB2 = 0x5; > > > psr_vsc.sdp_header.HB3 = 0x13; > > > - } else if (dev_priv->psr.y_cord_support) { > > > + } else { > > > psr_vsc.sdp_header.HB2 = 0x4; > > > psr_vsc.sdp_header.HB3 = 0xe; > > > - } else { > > > - psr_vsc.sdp_header.HB2 = 0x3; > > > - psr_vsc.sdp_header.HB3 = 0xc; > > > } > > > } else { > > > /* Prepare VSC packet as per EDP 1.3 spec, Table > > > 3.10 */ > > > @@ -458,15 +461,6 @@ static bool intel_psr2_config_valid(struct > > > intel_dp *intel_dp, > > > return false; > > > } > > > > > > - /* > > > - * FIXME:enable psr2 only for y-cordinate psr2 panels > > > - * After gtc implementation , remove this restriction. > > > - */ > > > - if (!dev_priv->psr.y_cord_support) { > > > - DRM_DEBUG_KMS("PSR2 not enabled, panel does not > > > support Y coordinate\n"); > > > - return false; > > > - } > > > - > > > return true; > > > } > > > > > > @@ -566,7 +560,6 @@ static void hsw_psr_enable_source(struct > > > intel_dp *intel_dp, > > > struct drm_device *dev = dig_port->base.base.dev; > > > struct drm_i915_private *dev_priv = to_i915(dev); > > > enum transcoder cpu_transcoder = crtc_state- > > > >cpu_transcoder; > > > - u32 chicken; > > > > > > psr_aux_io_power_get(intel_dp); > > > > > > @@ -577,9 +570,8 @@ static void hsw_psr_enable_source(struct > > > intel_dp *intel_dp, > > > hsw_psr_setup_aux(intel_dp); > > > > > > if (dev_priv->psr.psr2_support) { > > > - chicken = PSR2_VSC_ENABLE_PROG_HEADER; > > > - if (dev_priv->psr.y_cord_support) > > > - chicken |= PSR2_ADD_VERTICAL_LINE_COUNT; > > > + u32 chicken = PSR2_VSC_ENABLE_PROG_HEADER > > > + | PSR2_ADD_VERTICAL_LINE_COUNT; > > > I915_WRITE(CHICKEN_TRANS(cpu_transcoder), > > > chicken); > > > > > > I915_WRITE(EDP_PSR_DEBUG, > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Fri, Mar 23, 2018 at 07:34:34PM -0700, Pandiyan, Dhinakaran wrote: > > On Fri, 2018-03-23 at 23:51 +0000, Souza, Jose wrote: > > On Fri, 2018-03-23 at 15:59 -0700, Pandiyan, Dhinakaran wrote: > > > > > > > > > On Thu, 2018-03-22 at 14:48 -0700, José Roberto de Souza wrote: > > > > Move to only one place the sink requirements that the actual driver > > > > needs to enable PSR2. > > > > > > > > Also intel_psr2_config_valid() is called every time the crtc config > > > > is computed, wasting some time every time it was checking for > > > > Y coordinate requirement. > > > > > > > > This allow us to nuke y_cord_support and some of VSC setup code > > > > that > > > > was handling a scenario that would never happen(PSR2 without Y > > > > coordinate). > > > > > > > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > > > > --- > > > > drivers/gpu/drm/i915/i915_drv.h | 1 - > > > > drivers/gpu/drm/i915/intel_psr.c | 46 +++++++++++++++++----------- > > > > ------------ > > > > 2 files changed, 19 insertions(+), 28 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h > > > > b/drivers/gpu/drm/i915/i915_drv.h > > > > index 7fe00509e51a..cce32686fd75 100644 > > > > --- a/drivers/gpu/drm/i915/i915_drv.h > > > > +++ b/drivers/gpu/drm/i915/i915_drv.h > > > > @@ -603,7 +603,6 @@ struct i915_psr { > > > > unsigned busy_frontbuffer_bits; > > > > bool psr2_support; > > > > bool link_standby; > > > > - bool y_cord_support; > > > > bool colorimetry_support; > > > > bool alpm; > > > > bool has_hw_tracking; > > > > diff --git a/drivers/gpu/drm/i915/intel_psr.c > > > > b/drivers/gpu/drm/i915/intel_psr.c > > > > index d46320a451d9..23f38ab10636 100644 > > > > --- a/drivers/gpu/drm/i915/intel_psr.c > > > > +++ b/drivers/gpu/drm/i915/intel_psr.c > > > > @@ -93,7 +93,7 @@ static void psr_aux_io_power_put(struct intel_dp > > > > *intel_dp) > > > > intel_display_power_put(dev_priv, > > > > psr_aux_domain(intel_dp)); > > > > } > > > > > > > > -static bool intel_dp_get_y_cord_status(struct intel_dp *intel_dp) > > > > +static bool intel_dp_get_y_coord_required(struct intel_dp > > > > *intel_dp) > > > > { > > > > uint8_t psr_caps = 0; > > > > > > > > @@ -130,22 +130,29 @@ void intel_psr_init_dpcd(struct intel_dp > > > > *intel_dp) > > > > drm_dp_dpcd_read(&intel_dp->aux, DP_PSR_SUPPORT, intel_dp- > > > > >psr_dpcd, > > > > sizeof(intel_dp->psr_dpcd)); > > > > > > > > - if (intel_dp->psr_dpcd[0] & DP_PSR_IS_SUPPORTED) { > > > > + if (intel_dp->psr_dpcd[0]) { > > > > dev_priv->psr.sink_support = true; > > > > DRM_DEBUG_KMS("Detected EDP PSR Panel.\n"); > > > > } > > > > > > > > if (INTEL_GEN(dev_priv) >= 9 && > > > > - (intel_dp->psr_dpcd[0] & DP_PSR2_IS_SUPPORTED)) { > > > > - > > > > - dev_priv->psr.sink_support = true; > > > > - dev_priv->psr.psr2_support = true; > > > > + (intel_dp->psr_dpcd[0] == > > > > DP_PSR2_WITH_Y_COORD_IS_SUPPORTED)) { > > > > + /* > > > > + * All panels that supports PSR version 03h (PSR2 > > > > + > > > > + * Y-coordinate) can handle Y-coordinates in VSC > > > > but we are > > > > + * only sure that it is going to be used when > > > > required by the > > > > + * panel. This way panel is capable to do > > > > selective update > > > > + * without a aux frame sync. > > > > > > Can you cite this from the spec please? just the "panel is capable to > > > do > > > selective update without a aux frame sync" part. Or you could just > > > remove that line so that we can get this patch reviewed and merged. > > > > There is no such statement in spec like this, the closest that it have > > is: > > "When the Source device includes the optional Y-coordinate in the SDP > > for PSR2 Operation, as described in Table 6-12, the Sink device can > > implement a lower-precision GTC Slave function, as described in Table > > 7-1." > > > > But we know that this works by all the previous tests when enabling > > PSR2 and I also tested it. > > Please do update the comment to say so. Do you know if these tests for > PSR2 selective update are in IGT? Or were these manual tests? > > Rodrigo/Vathsala, any idea how selective update was and can be tested? nope :( > > The patch as such makes sense to me > Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> if you > update the comment. > > > > > > > > > > > > > > + * > > > > + * To support PSR version 02h and PSR version 03h > > > > without > > > > + * Y-coordinate requirement panels we would need > > > > to enable > > > > + * GTC first. > > > > + */ > > > > + dev_priv->psr.psr2_support = > > > > intel_dp_get_y_coord_required(intel_dp); > > > > DRM_DEBUG_KMS("PSR2 %s on sink", > > > > dev_priv->psr.psr2_support ? > > > > "supported" : "not supported"); > > > > > > > > if (dev_priv->psr.psr2_support) { > > > > - dev_priv->psr.y_cord_support = > > > > - intel_dp_get_y_cord_status(intel_d > > > > p); > > > > dev_priv->psr.colorimetry_support = > > > > intel_dp_get_colorimetry_status(in > > > > tel_dp); > > > > dev_priv->psr.alpm = > > > > @@ -191,16 +198,12 @@ static void hsw_psr_setup_vsc(struct intel_dp > > > > *intel_dp, > > > > memset(&psr_vsc, 0, sizeof(psr_vsc)); > > > > psr_vsc.sdp_header.HB0 = 0; > > > > psr_vsc.sdp_header.HB1 = 0x7; > > > > - if (dev_priv->psr.colorimetry_support && > > > > - dev_priv->psr.y_cord_support) { > > > > + if (dev_priv->psr.colorimetry_support) { > > > > psr_vsc.sdp_header.HB2 = 0x5; > > > > psr_vsc.sdp_header.HB3 = 0x13; > > > > - } else if (dev_priv->psr.y_cord_support) { > > > > + } else { > > > > psr_vsc.sdp_header.HB2 = 0x4; > > > > psr_vsc.sdp_header.HB3 = 0xe; > > > > - } else { > > > > - psr_vsc.sdp_header.HB2 = 0x3; > > > > - psr_vsc.sdp_header.HB3 = 0xc; > > > > } > > > > } else { > > > > /* Prepare VSC packet as per EDP 1.3 spec, Table > > > > 3.10 */ > > > > @@ -458,15 +461,6 @@ static bool intel_psr2_config_valid(struct > > > > intel_dp *intel_dp, > > > > return false; > > > > } > > > > > > > > - /* > > > > - * FIXME:enable psr2 only for y-cordinate psr2 panels > > > > - * After gtc implementation , remove this restriction. > > > > - */ > > > > - if (!dev_priv->psr.y_cord_support) { > > > > - DRM_DEBUG_KMS("PSR2 not enabled, panel does not > > > > support Y coordinate\n"); > > > > - return false; > > > > - } > > > > - > > > > return true; > > > > } > > > > > > > > @@ -566,7 +560,6 @@ static void hsw_psr_enable_source(struct > > > > intel_dp *intel_dp, > > > > struct drm_device *dev = dig_port->base.base.dev; > > > > struct drm_i915_private *dev_priv = to_i915(dev); > > > > enum transcoder cpu_transcoder = crtc_state- > > > > >cpu_transcoder; > > > > - u32 chicken; > > > > > > > > psr_aux_io_power_get(intel_dp); > > > > > > > > @@ -577,9 +570,8 @@ static void hsw_psr_enable_source(struct > > > > intel_dp *intel_dp, > > > > hsw_psr_setup_aux(intel_dp); > > > > > > > > if (dev_priv->psr.psr2_support) { > > > > - chicken = PSR2_VSC_ENABLE_PROG_HEADER; > > > > - if (dev_priv->psr.y_cord_support) > > > > - chicken |= PSR2_ADD_VERTICAL_LINE_COUNT; > > > > + u32 chicken = PSR2_VSC_ENABLE_PROG_HEADER > > > > + | PSR2_ADD_VERTICAL_LINE_COUNT; > > > > I915_WRITE(CHICKEN_TRANS(cpu_transcoder), > > > > chicken); > > > > > > > > I915_WRITE(EDP_PSR_DEBUG, > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Selective update testing play a video and check 0x60094 , (03, 06) will indicated that system is in su state. -----Original Message----- From: Vivi, Rodrigo Sent: Wednesday, March 28, 2018 3:07 AM To: Pandiyan, Dhinakaran <dhinakaran.pandiyan@intel.com> Cc: Souza, Jose <jose.souza@intel.com>; intel-gfx@lists.freedesktop.org; Nagaraju, Vathsala <vathsala.nagaraju@intel.com> Subject: Re: [Intel-gfx] [PATCH 04/12] drm/i915/psr: Tie PSR2 support to Y coordinate requirement On Fri, Mar 23, 2018 at 07:34:34PM -0700, Pandiyan, Dhinakaran wrote: > > On Fri, 2018-03-23 at 23:51 +0000, Souza, Jose wrote: > > On Fri, 2018-03-23 at 15:59 -0700, Pandiyan, Dhinakaran wrote: > > > > > > > > > On Thu, 2018-03-22 at 14:48 -0700, José Roberto de Souza wrote: > > > > Move to only one place the sink requirements that the actual > > > > driver needs to enable PSR2. > > > > > > > > Also intel_psr2_config_valid() is called every time the crtc > > > > config is computed, wasting some time every time it was checking > > > > for Y coordinate requirement. > > > > > > > > This allow us to nuke y_cord_support and some of VSC setup code > > > > that was handling a scenario that would never happen(PSR2 > > > > without Y coordinate). > > > > > > > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > > > > --- > > > > drivers/gpu/drm/i915/i915_drv.h | 1 - > > > > drivers/gpu/drm/i915/intel_psr.c | 46 > > > > +++++++++++++++++----------- > > > > ------------ > > > > 2 files changed, 19 insertions(+), 28 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h > > > > b/drivers/gpu/drm/i915/i915_drv.h index > > > > 7fe00509e51a..cce32686fd75 100644 > > > > --- a/drivers/gpu/drm/i915/i915_drv.h > > > > +++ b/drivers/gpu/drm/i915/i915_drv.h > > > > @@ -603,7 +603,6 @@ struct i915_psr { > > > > unsigned busy_frontbuffer_bits; > > > > bool psr2_support; > > > > bool link_standby; > > > > - bool y_cord_support; > > > > bool colorimetry_support; > > > > bool alpm; > > > > bool has_hw_tracking; > > > > diff --git a/drivers/gpu/drm/i915/intel_psr.c > > > > b/drivers/gpu/drm/i915/intel_psr.c > > > > index d46320a451d9..23f38ab10636 100644 > > > > --- a/drivers/gpu/drm/i915/intel_psr.c > > > > +++ b/drivers/gpu/drm/i915/intel_psr.c > > > > @@ -93,7 +93,7 @@ static void psr_aux_io_power_put(struct > > > > intel_dp > > > > *intel_dp) > > > > intel_display_power_put(dev_priv, psr_aux_domain(intel_dp)); > > > > } > > > > > > > > -static bool intel_dp_get_y_cord_status(struct intel_dp > > > > *intel_dp) > > > > +static bool intel_dp_get_y_coord_required(struct intel_dp > > > > *intel_dp) > > > > { > > > > uint8_t psr_caps = 0; > > > > > > > > @@ -130,22 +130,29 @@ void intel_psr_init_dpcd(struct intel_dp > > > > *intel_dp) > > > > drm_dp_dpcd_read(&intel_dp->aux, DP_PSR_SUPPORT, intel_dp- > > > > >psr_dpcd, > > > > sizeof(intel_dp->psr_dpcd)); > > > > > > > > - if (intel_dp->psr_dpcd[0] & DP_PSR_IS_SUPPORTED) { > > > > + if (intel_dp->psr_dpcd[0]) { > > > > dev_priv->psr.sink_support = true; > > > > DRM_DEBUG_KMS("Detected EDP PSR Panel.\n"); > > > > } > > > > > > > > if (INTEL_GEN(dev_priv) >= 9 && > > > > - (intel_dp->psr_dpcd[0] & DP_PSR2_IS_SUPPORTED)) { > > > > - > > > > - dev_priv->psr.sink_support = true; > > > > - dev_priv->psr.psr2_support = true; > > > > + (intel_dp->psr_dpcd[0] == > > > > DP_PSR2_WITH_Y_COORD_IS_SUPPORTED)) { > > > > + /* > > > > + * All panels that supports PSR version 03h (PSR2 > > > > + > > > > + * Y-coordinate) can handle Y-coordinates in VSC > > > > but we are > > > > + * only sure that it is going to be used when > > > > required by the > > > > + * panel. This way panel is capable to do > > > > selective update > > > > + * without a aux frame sync. > > > > > > Can you cite this from the spec please? just the "panel is capable > > > to do selective update without a aux frame sync" part. Or you > > > could just remove that line so that we can get this patch reviewed > > > and merged. > > > > There is no such statement in spec like this, the closest that it > > have > > is: > > "When the Source device includes the optional Y-coordinate in the > > SDP for PSR2 Operation, as described in Table 6-12, the Sink device > > can implement a lower-precision GTC Slave function, as described in > > Table 7-1." > > > > But we know that this works by all the previous tests when enabling > > PSR2 and I also tested it. > > Please do update the comment to say so. Do you know if these tests for > PSR2 selective update are in IGT? Or were these manual tests? > > Rodrigo/Vathsala, any idea how selective update was and can be tested? nope :( > > The patch as such makes sense to me > Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> if > you update the comment. > > > > > > > > > > > > > > + * > > > > + * To support PSR version 02h and PSR version 03h > > > > without > > > > + * Y-coordinate requirement panels we would need > > > > to enable > > > > + * GTC first. > > > > + */ > > > > + dev_priv->psr.psr2_support = > > > > intel_dp_get_y_coord_required(intel_dp); > > > > DRM_DEBUG_KMS("PSR2 %s on sink", > > > > dev_priv->psr.psr2_support ? > > > > "supported" : "not supported"); > > > > > > > > if (dev_priv->psr.psr2_support) { > > > > - dev_priv->psr.y_cord_support = > > > > - intel_dp_get_y_cord_status(intel_d > > > > p); > > > > dev_priv->psr.colorimetry_support = > > > > intel_dp_get_colorimetry_status(in > > > > tel_dp); > > > > dev_priv->psr.alpm = > > > > @@ -191,16 +198,12 @@ static void hsw_psr_setup_vsc(struct > > > > intel_dp *intel_dp, > > > > memset(&psr_vsc, 0, sizeof(psr_vsc)); > > > > psr_vsc.sdp_header.HB0 = 0; > > > > psr_vsc.sdp_header.HB1 = 0x7; > > > > - if (dev_priv->psr.colorimetry_support && > > > > - dev_priv->psr.y_cord_support) { > > > > + if (dev_priv->psr.colorimetry_support) { > > > > psr_vsc.sdp_header.HB2 = 0x5; > > > > psr_vsc.sdp_header.HB3 = 0x13; > > > > - } else if (dev_priv->psr.y_cord_support) { > > > > + } else { > > > > psr_vsc.sdp_header.HB2 = 0x4; > > > > psr_vsc.sdp_header.HB3 = 0xe; > > > > - } else { > > > > - psr_vsc.sdp_header.HB2 = 0x3; > > > > - psr_vsc.sdp_header.HB3 = 0xc; > > > > } > > > > } else { > > > > /* Prepare VSC packet as per EDP 1.3 spec, Table > > > > 3.10 */ > > > > @@ -458,15 +461,6 @@ static bool intel_psr2_config_valid(struct > > > > intel_dp *intel_dp, > > > > return false; > > > > } > > > > > > > > - /* > > > > - * FIXME:enable psr2 only for y-cordinate psr2 panels > > > > - * After gtc implementation , remove this restriction. > > > > - */ > > > > - if (!dev_priv->psr.y_cord_support) { > > > > - DRM_DEBUG_KMS("PSR2 not enabled, panel does not > > > > support Y coordinate\n"); > > > > - return false; > > > > - } > > > > - > > > > return true; > > > > } > > > > > > > > @@ -566,7 +560,6 @@ static void hsw_psr_enable_source(struct > > > > intel_dp *intel_dp, > > > > struct drm_device *dev = dig_port->base.base.dev; > > > > struct drm_i915_private *dev_priv = to_i915(dev); > > > > enum transcoder cpu_transcoder = crtc_state- > > > > >cpu_transcoder; > > > > - u32 chicken; > > > > > > > > psr_aux_io_power_get(intel_dp); > > > > > > > > @@ -577,9 +570,8 @@ static void hsw_psr_enable_source(struct > > > > intel_dp *intel_dp, > > > > hsw_psr_setup_aux(intel_dp); > > > > > > > > if (dev_priv->psr.psr2_support) { > > > > - chicken = PSR2_VSC_ENABLE_PROG_HEADER; > > > > - if (dev_priv->psr.y_cord_support) > > > > - chicken |= PSR2_ADD_VERTICAL_LINE_COUNT; > > > > + u32 chicken = PSR2_VSC_ENABLE_PROG_HEADER > > > > + | PSR2_ADD_VERTICAL_LINE_COUNT; > > > > I915_WRITE(CHICKEN_TRANS(cpu_transcoder), > > > > chicken); > > > > > > > > I915_WRITE(EDP_PSR_DEBUG, > > _______________________________________________ > > 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 7fe00509e51a..cce32686fd75 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -603,7 +603,6 @@ struct i915_psr { unsigned busy_frontbuffer_bits; bool psr2_support; bool link_standby; - bool y_cord_support; bool colorimetry_support; bool alpm; bool has_hw_tracking; diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c index d46320a451d9..23f38ab10636 100644 --- a/drivers/gpu/drm/i915/intel_psr.c +++ b/drivers/gpu/drm/i915/intel_psr.c @@ -93,7 +93,7 @@ static void psr_aux_io_power_put(struct intel_dp *intel_dp) intel_display_power_put(dev_priv, psr_aux_domain(intel_dp)); } -static bool intel_dp_get_y_cord_status(struct intel_dp *intel_dp) +static bool intel_dp_get_y_coord_required(struct intel_dp *intel_dp) { uint8_t psr_caps = 0; @@ -130,22 +130,29 @@ void intel_psr_init_dpcd(struct intel_dp *intel_dp) drm_dp_dpcd_read(&intel_dp->aux, DP_PSR_SUPPORT, intel_dp->psr_dpcd, sizeof(intel_dp->psr_dpcd)); - if (intel_dp->psr_dpcd[0] & DP_PSR_IS_SUPPORTED) { + if (intel_dp->psr_dpcd[0]) { dev_priv->psr.sink_support = true; DRM_DEBUG_KMS("Detected EDP PSR Panel.\n"); } if (INTEL_GEN(dev_priv) >= 9 && - (intel_dp->psr_dpcd[0] & DP_PSR2_IS_SUPPORTED)) { - - dev_priv->psr.sink_support = true; - dev_priv->psr.psr2_support = true; + (intel_dp->psr_dpcd[0] == DP_PSR2_WITH_Y_COORD_IS_SUPPORTED)) { + /* + * All panels that supports PSR version 03h (PSR2 + + * Y-coordinate) can handle Y-coordinates in VSC but we are + * only sure that it is going to be used when required by the + * panel. This way panel is capable to do selective update + * without a aux frame sync. + * + * To support PSR version 02h and PSR version 03h without + * Y-coordinate requirement panels we would need to enable + * GTC first. + */ + dev_priv->psr.psr2_support = intel_dp_get_y_coord_required(intel_dp); DRM_DEBUG_KMS("PSR2 %s on sink", dev_priv->psr.psr2_support ? "supported" : "not supported"); if (dev_priv->psr.psr2_support) { - dev_priv->psr.y_cord_support = - intel_dp_get_y_cord_status(intel_dp); dev_priv->psr.colorimetry_support = intel_dp_get_colorimetry_status(intel_dp); dev_priv->psr.alpm = @@ -191,16 +198,12 @@ static void hsw_psr_setup_vsc(struct intel_dp *intel_dp, memset(&psr_vsc, 0, sizeof(psr_vsc)); psr_vsc.sdp_header.HB0 = 0; psr_vsc.sdp_header.HB1 = 0x7; - if (dev_priv->psr.colorimetry_support && - dev_priv->psr.y_cord_support) { + if (dev_priv->psr.colorimetry_support) { psr_vsc.sdp_header.HB2 = 0x5; psr_vsc.sdp_header.HB3 = 0x13; - } else if (dev_priv->psr.y_cord_support) { + } else { psr_vsc.sdp_header.HB2 = 0x4; psr_vsc.sdp_header.HB3 = 0xe; - } else { - psr_vsc.sdp_header.HB2 = 0x3; - psr_vsc.sdp_header.HB3 = 0xc; } } else { /* Prepare VSC packet as per EDP 1.3 spec, Table 3.10 */ @@ -458,15 +461,6 @@ static bool intel_psr2_config_valid(struct intel_dp *intel_dp, return false; } - /* - * FIXME:enable psr2 only for y-cordinate psr2 panels - * After gtc implementation , remove this restriction. - */ - if (!dev_priv->psr.y_cord_support) { - DRM_DEBUG_KMS("PSR2 not enabled, panel does not support Y coordinate\n"); - return false; - } - return true; } @@ -566,7 +560,6 @@ static void hsw_psr_enable_source(struct intel_dp *intel_dp, struct drm_device *dev = dig_port->base.base.dev; struct drm_i915_private *dev_priv = to_i915(dev); enum transcoder cpu_transcoder = crtc_state->cpu_transcoder; - u32 chicken; psr_aux_io_power_get(intel_dp); @@ -577,9 +570,8 @@ static void hsw_psr_enable_source(struct intel_dp *intel_dp, hsw_psr_setup_aux(intel_dp); if (dev_priv->psr.psr2_support) { - chicken = PSR2_VSC_ENABLE_PROG_HEADER; - if (dev_priv->psr.y_cord_support) - chicken |= PSR2_ADD_VERTICAL_LINE_COUNT; + u32 chicken = PSR2_VSC_ENABLE_PROG_HEADER + | PSR2_ADD_VERTICAL_LINE_COUNT; I915_WRITE(CHICKEN_TRANS(cpu_transcoder), chicken); I915_WRITE(EDP_PSR_DEBUG,
Move to only one place the sink requirements that the actual driver needs to enable PSR2. Also intel_psr2_config_valid() is called every time the crtc config is computed, wasting some time every time it was checking for Y coordinate requirement. This allow us to nuke y_cord_support and some of VSC setup code that was handling a scenario that would never happen(PSR2 without Y coordinate). Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> Signed-off-by: José Roberto de Souza <jose.souza@intel.com> --- drivers/gpu/drm/i915/i915_drv.h | 1 - drivers/gpu/drm/i915/intel_psr.c | 46 +++++++++++++++++----------------------- 2 files changed, 19 insertions(+), 28 deletions(-)