| Message ID | 1533071239-28815-14-git-send-email-manasi.d.navare@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Display Stream Compression enabling on eDP/DP | expand |
On Tue, Jul 31, 2018 at 02:07:09PM -0700, Manasi Navare wrote: > If a eDP panel supports both PSR2 and VDSC, our HW cannot > support both at a time. Give priority to PSR2 if a requested > resolution can be supported without compression else enable > VDSC and keep PSR2 disabled. what about PSR1 on PSR2 panels? could it be enabled with VSC? or is there any restriction? > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > Cc: Jani Nikula <jani.nikula@intel.com> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> > --- > drivers/gpu/drm/i915/intel_psr.c | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c > index 4bd5768..fdb028f 100644 > --- a/drivers/gpu/drm/i915/intel_psr.c > +++ b/drivers/gpu/drm/i915/intel_psr.c > @@ -441,6 +441,16 @@ static bool intel_psr2_config_valid(struct intel_dp *intel_dp, > if (!dev_priv->psr.sink_psr2_support) > return false; > > + /* > + * DSC and PSR2 cannot be enabled simultaneously. If a requested > + * resolution requires DSC to be enabled, priority is given to DSC > + * over PSR2. > + */ > + if (crtc_state->dsc_params.compression_enable) { > + DRM_DEBUG_KMS("PSR2 cannot be enabled since DSC is enabled\n"); > + return false; > + } one concern I had when I first saw this patch is the order, but I saw that psr compute config is the last one inside dp compute config, so we are good... only "concern" now is about PSR1 restrictions. But if not restriction with PSR1 and VSC: Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > + > if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv)) { > psr_max_h = 4096; > psr_max_v = 2304; > -- > 2.7.4 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Thanks Rodrigo for your review. Please find the answers below: On Wed, Aug 08, 2018 at 10:55:13PM -0700, Rodrigo Vivi wrote: > On Tue, Jul 31, 2018 at 02:07:09PM -0700, Manasi Navare wrote: > > If a eDP panel supports both PSR2 and VDSC, our HW cannot > > support both at a time. Give priority to PSR2 if a requested > > resolution can be supported without compression else enable > > VDSC and keep PSR2 disabled. > > what about PSR1 on PSR2 panels? could it be enabled with VSC? > or is there any restriction? > PSR1 can be enabled simultaneously with VDSC. Its only the PSR2 with selective updates that cannot work along with DSC. Manasi > > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > Cc: Jani Nikula <jani.nikula@intel.com> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> > > --- > > drivers/gpu/drm/i915/intel_psr.c | 10 ++++++++++ > > 1 file changed, 10 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c > > index 4bd5768..fdb028f 100644 > > --- a/drivers/gpu/drm/i915/intel_psr.c > > +++ b/drivers/gpu/drm/i915/intel_psr.c > > @@ -441,6 +441,16 @@ static bool intel_psr2_config_valid(struct intel_dp *intel_dp, > > if (!dev_priv->psr.sink_psr2_support) > > return false; > > > > + /* > > + * DSC and PSR2 cannot be enabled simultaneously. If a requested > > + * resolution requires DSC to be enabled, priority is given to DSC > > + * over PSR2. > > + */ > > + if (crtc_state->dsc_params.compression_enable) { > > + DRM_DEBUG_KMS("PSR2 cannot be enabled since DSC is enabled\n"); > > + return false; > > + } > > one concern I had when I first saw this patch is the order, but > I saw that psr compute config is the last one inside dp compute config, > so we are good... > > only "concern" now is about PSR1 restrictions. > > But if not restriction with PSR1 and VSC: > > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > > + > > if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv)) { > > psr_max_h = 4096; > > psr_max_v = 2304; > > -- > > 2.7.4 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Thu, Aug 23, 2018 at 12:22:29PM -0700, Manasi Navare wrote: > Thanks Rodrigo for your review. Please find the answers below: > > On Wed, Aug 08, 2018 at 10:55:13PM -0700, Rodrigo Vivi wrote: > > On Tue, Jul 31, 2018 at 02:07:09PM -0700, Manasi Navare wrote: > > > If a eDP panel supports both PSR2 and VDSC, our HW cannot > > > support both at a time. Give priority to PSR2 if a requested > > > resolution can be supported without compression else enable > > > VDSC and keep PSR2 disabled. > > > > what about PSR1 on PSR2 panels? could it be enabled with VSC? > > or is there any restriction? > > > > PSR1 can be enabled simultaneously with VDSC. Its only the PSR2 with selective updates > that cannot work along with DSC. cool, thanks for the confirmation.... feel free to use Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > Manasi > > > > > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > Cc: Jani Nikula <jani.nikula@intel.com> > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> > > > --- > > > drivers/gpu/drm/i915/intel_psr.c | 10 ++++++++++ > > > 1 file changed, 10 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c > > > index 4bd5768..fdb028f 100644 > > > --- a/drivers/gpu/drm/i915/intel_psr.c > > > +++ b/drivers/gpu/drm/i915/intel_psr.c > > > @@ -441,6 +441,16 @@ static bool intel_psr2_config_valid(struct intel_dp *intel_dp, > > > if (!dev_priv->psr.sink_psr2_support) > > > return false; > > > > > > + /* > > > + * DSC and PSR2 cannot be enabled simultaneously. If a requested > > > + * resolution requires DSC to be enabled, priority is given to DSC > > > + * over PSR2. > > > + */ > > > + if (crtc_state->dsc_params.compression_enable) { > > > + DRM_DEBUG_KMS("PSR2 cannot be enabled since DSC is enabled\n"); > > > + return false; > > > + } > > > > one concern I had when I first saw this patch is the order, but > > I saw that psr compute config is the last one inside dp compute config, > > so we are good... > > > > only "concern" now is about PSR1 restrictions. > > > > But if not restriction with PSR1 and VSC: > > > > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > > > > > + > > > if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv)) { > > > psr_max_h = 4096; > > > psr_max_v = 2304; > > > -- > > > 2.7.4 > > > > > > _______________________________________________ > > > Intel-gfx mailing list > > > Intel-gfx@lists.freedesktop.org > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Wed, 2018-08-08 at 22:55 -0700, Rodrigo Vivi wrote: > On Tue, Jul 31, 2018 at 02:07:09PM -0700, Manasi Navare wrote: > > If a eDP panel supports both PSR2 and VDSC, our HW cannot > > support both at a time. Give priority to PSR2 if a requested > > resolution can be supported without compression else enable > > VDSC and keep PSR2 disabled. > > what about PSR1 on PSR2 panels? could it be enabled with VSC? > or is there any restriction? > > > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > Cc: Jani Nikula <jani.nikula@intel.com> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> > > --- > > drivers/gpu/drm/i915/intel_psr.c | 10 ++++++++++ > > 1 file changed, 10 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/intel_psr.c > > b/drivers/gpu/drm/i915/intel_psr.c > > index 4bd5768..fdb028f 100644 > > --- a/drivers/gpu/drm/i915/intel_psr.c > > +++ b/drivers/gpu/drm/i915/intel_psr.c > > @@ -441,6 +441,16 @@ static bool intel_psr2_config_valid(struct > > intel_dp *intel_dp, > > if (!dev_priv->psr.sink_psr2_support) > > return false; > > > > + /* > > + * DSC and PSR2 cannot be enabled simultaneously. If a > > requested > > + * resolution requires DSC to be enabled, priority is > > given to DSC > > + * over PSR2. > > + */ > > + if (crtc_state->dsc_params.compression_enable) { > > + DRM_DEBUG_KMS("PSR2 cannot be enabled since DSC is > > enabled\n"); > > + return false; > > + } > > one concern I had when I first saw this patch is the order, but > I saw that psr compute config is the last one inside dp compute > config, Do something more to ensure that order stays as it is? A warn if both get enabled, or a comment in intel_dp.c perhaps. That is what we do for drrs and psr. > so we are good... > > only "concern" now is about PSR1 restrictions. > > But if not restriction with PSR1 and VSC: > > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > > + > > if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv)) > > { > > psr_max_h = 4096; > > psr_max_v = 2304; > > -- > > 2.7.4 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Thu, Aug 23, 2018 at 01:34:54PM -0700, Dhinakaran Pandiyan wrote: > > > On Wed, 2018-08-08 at 22:55 -0700, Rodrigo Vivi wrote: > > On Tue, Jul 31, 2018 at 02:07:09PM -0700, Manasi Navare wrote: > > > If a eDP panel supports both PSR2 and VDSC, our HW cannot > > > support both at a time. Give priority to PSR2 if a requested > > > resolution can be supported without compression else enable > > > VDSC and keep PSR2 disabled. > > > > what about PSR1 on PSR2 panels? could it be enabled with VSC? > > or is there any restriction? > > > > > > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > Cc: Jani Nikula <jani.nikula@intel.com> > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> > > > --- > > > drivers/gpu/drm/i915/intel_psr.c | 10 ++++++++++ > > > 1 file changed, 10 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_psr.c > > > b/drivers/gpu/drm/i915/intel_psr.c > > > index 4bd5768..fdb028f 100644 > > > --- a/drivers/gpu/drm/i915/intel_psr.c > > > +++ b/drivers/gpu/drm/i915/intel_psr.c > > > @@ -441,6 +441,16 @@ static bool intel_psr2_config_valid(struct > > > intel_dp *intel_dp, > > > if (!dev_priv->psr.sink_psr2_support) > > > return false; > > > > > > + /* > > > + * DSC and PSR2 cannot be enabled simultaneously. If a > > > requested > > > + * resolution requires DSC to be enabled, priority is > > > given to DSC > > > + * over PSR2. > > > + */ > > > + if (crtc_state->dsc_params.compression_enable) { > > > + DRM_DEBUG_KMS("PSR2 cannot be enabled since DSC is > > > enabled\n"); > > > + return false; > > > + } > > > > one concern I had when I first saw this patch is the order, but > > I saw that psr compute config is the last one inside dp compute > > config, > Do something more to ensure that order stays as it is? A warn if both > get enabled, or a comment in intel_dp.c perhaps. That is what we do for > drrs and psr. hmm... that's a good idea. > > > so we are good... > > > > only "concern" now is about PSR1 restrictions. > > > > But if not restriction with PSR1 and VSC: > > > > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > > > > > + > > > if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv)) > > > { > > > psr_max_h = 4096; > > > psr_max_v = 2304; > > > -- > > > 2.7.4 > > > > > > _______________________________________________ > > > Intel-gfx mailing list > > > Intel-gfx@lists.freedesktop.org > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Thu, Aug 23, 2018 at 01:57:46PM -0700, Rodrigo Vivi wrote: > On Thu, Aug 23, 2018 at 01:34:54PM -0700, Dhinakaran Pandiyan wrote: > > > > > > On Wed, 2018-08-08 at 22:55 -0700, Rodrigo Vivi wrote: > > > On Tue, Jul 31, 2018 at 02:07:09PM -0700, Manasi Navare wrote: > > > > If a eDP panel supports both PSR2 and VDSC, our HW cannot > > > > support both at a time. Give priority to PSR2 if a requested > > > > resolution can be supported without compression else enable > > > > VDSC and keep PSR2 disabled. > > > > > > what about PSR1 on PSR2 panels? could it be enabled with VSC? > > > or is there any restriction? > > > > > > > > > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > > Cc: Jani Nikula <jani.nikula@intel.com> > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> > > > > --- > > > > drivers/gpu/drm/i915/intel_psr.c | 10 ++++++++++ > > > > 1 file changed, 10 insertions(+) > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_psr.c > > > > b/drivers/gpu/drm/i915/intel_psr.c > > > > index 4bd5768..fdb028f 100644 > > > > --- a/drivers/gpu/drm/i915/intel_psr.c > > > > +++ b/drivers/gpu/drm/i915/intel_psr.c > > > > @@ -441,6 +441,16 @@ static bool intel_psr2_config_valid(struct > > > > intel_dp *intel_dp, > > > > if (!dev_priv->psr.sink_psr2_support) > > > > return false; > > > > > > > > + /* > > > > + * DSC and PSR2 cannot be enabled simultaneously. If a > > > > requested > > > > + * resolution requires DSC to be enabled, priority is > > > > given to DSC > > > > + * over PSR2. > > > > + */ > > > > + if (crtc_state->dsc_params.compression_enable) { > > > > + DRM_DEBUG_KMS("PSR2 cannot be enabled since DSC is > > > > enabled\n"); > > > > + return false; > > > > + } > > > > > > one concern I had when I first saw this patch is the order, but > > > I saw that psr compute config is the last one inside dp compute > > > config, > > Do something more to ensure that order stays as it is? A warn if both > > get enabled, or a comment in intel_dp.c perhaps. That is what we do for > > drrs and psr. > > hmm... that's a good idea. Are you suggesting a Warning after psr_compute_config if psr and dsc both are enabled? Manasi > > > > > > so we are good... > > > > > > only "concern" now is about PSR1 restrictions. > > > > > > But if not restriction with PSR1 and VSC: > > > > > > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > > > > > > > > + > > > > if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv)) > > > > { > > > > psr_max_h = 4096; > > > > psr_max_v = 2304; > > > > -- > > > > 2.7.4 > > > > > > > > _______________________________________________ > > > > Intel-gfx mailing list > > > > Intel-gfx@lists.freedesktop.org > > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > > > _______________________________________________ > > > Intel-gfx mailing list > > > Intel-gfx@lists.freedesktop.org > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c index 4bd5768..fdb028f 100644 --- a/drivers/gpu/drm/i915/intel_psr.c +++ b/drivers/gpu/drm/i915/intel_psr.c @@ -441,6 +441,16 @@ static bool intel_psr2_config_valid(struct intel_dp *intel_dp, if (!dev_priv->psr.sink_psr2_support) return false; + /* + * DSC and PSR2 cannot be enabled simultaneously. If a requested + * resolution requires DSC to be enabled, priority is given to DSC + * over PSR2. + */ + if (crtc_state->dsc_params.compression_enable) { + DRM_DEBUG_KMS("PSR2 cannot be enabled since DSC is enabled\n"); + return false; + } + if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv)) { psr_max_h = 4096; psr_max_v = 2304;
If a eDP panel supports both PSR2 and VDSC, our HW cannot support both at a time. Give priority to PSR2 if a requested resolution can be supported without compression else enable VDSC and keep PSR2 disabled. Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> Cc: Jani Nikula <jani.nikula@intel.com> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> --- drivers/gpu/drm/i915/intel_psr.c | 10 ++++++++++ 1 file changed, 10 insertions(+)