| Message ID | 20190326144903.6617-1-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] drm/i915: Do not enable FEC without DSC | expand |
On Tue, Mar 26, 2019 at 04:49:02PM +0200, Ville Syrjala wrote: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Currently we enable FEC even when DSC is no used. While that is > theoretically valid supposedly there isn't much of a benefit from > this. But more importantly we do not account for the FEC link > bandwidth overhead (2.4%) in the non-DSC link bandwidth computations. > So the code may think we have enough bandwidth when we in fact > do not. > > Cc: Anusha Srivatsa <anusha.srivatsa@intel.com> > Cc: Manasi Navare <manasi.d.navare@intel.comk> > Fixes: 240999cf339f ("i915/dp/fec: Add fec_enable to the crtc state.") > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > drivers/gpu/drm/i915/intel_dp.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index 326de12c3f44..bbf678561509 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -1925,6 +1925,9 @@ static int intel_dp_dsc_compute_config(struct intel_dp *intel_dp, > int pipe_bpp; > int ret; > > + pipe_config->fec_enable = !intel_dp_is_edp(intel_dp) && > + intel_dp_supports_fec(intel_dp, pipe_config); > + We could still not enable DSC after this point since it has more checks in this function. Even though in that case we would fail the encoder config so wouldnt matter if we have enabled FEC or not, but its less intutive. IMHO, the ideal place to set the fec enable is in intel_dp_compute_link_config() after the all to dsc_compute_config and set it only if pipe_config->dsc_params.compression_enable Manasi > if (!intel_dp_supports_dsc(intel_dp, pipe_config)) > return -EINVAL; > > @@ -2168,9 +2171,6 @@ intel_dp_compute_config(struct intel_encoder *encoder, > if (adjusted_mode->flags & DRM_MODE_FLAG_DBLCLK) > return -EINVAL; > > - pipe_config->fec_enable = !intel_dp_is_edp(intel_dp) && > - intel_dp_supports_fec(intel_dp, pipe_config); > - > ret = intel_dp_compute_link_config(encoder, pipe_config, conn_state); > if (ret < 0) > return ret; > -- > 2.19.2 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Tue, Mar 26, 2019 at 09:00:27AM -0700, Manasi Navare wrote: > On Tue, Mar 26, 2019 at 04:49:02PM +0200, Ville Syrjala wrote: > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > Currently we enable FEC even when DSC is no used. While that is > > theoretically valid supposedly there isn't much of a benefit from > > this. But more importantly we do not account for the FEC link > > bandwidth overhead (2.4%) in the non-DSC link bandwidth computations. > > So the code may think we have enough bandwidth when we in fact > > do not. > > > > Cc: Anusha Srivatsa <anusha.srivatsa@intel.com> > > Cc: Manasi Navare <manasi.d.navare@intel.comk> > > Fixes: 240999cf339f ("i915/dp/fec: Add fec_enable to the crtc state.") > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > --- > > drivers/gpu/drm/i915/intel_dp.c | 6 +++--- > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > > index 326de12c3f44..bbf678561509 100644 > > --- a/drivers/gpu/drm/i915/intel_dp.c > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > @@ -1925,6 +1925,9 @@ static int intel_dp_dsc_compute_config(struct intel_dp *intel_dp, > > int pipe_bpp; > > int ret; > > > > + pipe_config->fec_enable = !intel_dp_is_edp(intel_dp) && > > + intel_dp_supports_fec(intel_dp, pipe_config); > > + > > We could still not enable DSC after this point since it has more checks in this > function. Even though in that case we would fail the encoder config so wouldnt > matter if we have enabled FEC or not, but its less intutive. > IMHO, the ideal place to set the fec enable is in intel_dp_compute_link_config() > after the all to dsc_compute_config and set it only if pipe_config->dsc_params.compression_enable That would require changing intel_dp_supports_dsc() which I decided wasn't worth the hassle. > > Manasi > > > if (!intel_dp_supports_dsc(intel_dp, pipe_config)) > > return -EINVAL; > > > > @@ -2168,9 +2171,6 @@ intel_dp_compute_config(struct intel_encoder *encoder, > > if (adjusted_mode->flags & DRM_MODE_FLAG_DBLCLK) > > return -EINVAL; > > > > - pipe_config->fec_enable = !intel_dp_is_edp(intel_dp) && > > - intel_dp_supports_fec(intel_dp, pipe_config); > > - > > ret = intel_dp_compute_link_config(encoder, pipe_config, conn_state); > > if (ret < 0) > > return ret; > > -- > > 2.19.2 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Tue, Mar 26, 2019 at 06:16:57PM +0200, Ville Syrjälä wrote: > On Tue, Mar 26, 2019 at 09:00:27AM -0700, Manasi Navare wrote: > > On Tue, Mar 26, 2019 at 04:49:02PM +0200, Ville Syrjala wrote: > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > > Currently we enable FEC even when DSC is no used. While that is > > > theoretically valid supposedly there isn't much of a benefit from > > > this. But more importantly we do not account for the FEC link > > > bandwidth overhead (2.4%) in the non-DSC link bandwidth computations. > > > So the code may think we have enough bandwidth when we in fact > > > do not. > > > > > > Cc: Anusha Srivatsa <anusha.srivatsa@intel.com> > > > Cc: Manasi Navare <manasi.d.navare@intel.comk> > > > Fixes: 240999cf339f ("i915/dp/fec: Add fec_enable to the crtc state.") > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > --- > > > drivers/gpu/drm/i915/intel_dp.c | 6 +++--- > > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > > > index 326de12c3f44..bbf678561509 100644 > > > --- a/drivers/gpu/drm/i915/intel_dp.c > > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > > @@ -1925,6 +1925,9 @@ static int intel_dp_dsc_compute_config(struct intel_dp *intel_dp, > > > int pipe_bpp; > > > int ret; > > > > > > + pipe_config->fec_enable = !intel_dp_is_edp(intel_dp) && > > > + intel_dp_supports_fec(intel_dp, pipe_config); > > > + > > > > We could still not enable DSC after this point since it has more checks in this > > function. Even though in that case we would fail the encoder config so wouldnt > > matter if we have enabled FEC or not, but its less intutive. > > IMHO, the ideal place to set the fec enable is in intel_dp_compute_link_config() > > after the all to dsc_compute_config and set it only if pipe_config->dsc_params.compression_enable > > That would require changing intel_dp_supports_dsc() which I decided > wasn't worth the hassle. Hmm, yea because intel_dp_supports_dsc depends on fec_enable. In that case we would need to change that to use intel_dp_supports_fec(). TBH that makes more sense since we should set dp_supports based on HW FEC capability and then set the crtc_state->fec_enable based on crtc_state->dsc_params.compression_enable. But since it doesnt change the functionality, I am ok either ways. Manasi > > > > > Manasi > > > > > if (!intel_dp_supports_dsc(intel_dp, pipe_config)) > > > return -EINVAL; > > > > > > @@ -2168,9 +2171,6 @@ intel_dp_compute_config(struct intel_encoder *encoder, > > > if (adjusted_mode->flags & DRM_MODE_FLAG_DBLCLK) > > > return -EINVAL; > > > > > > - pipe_config->fec_enable = !intel_dp_is_edp(intel_dp) && > > > - intel_dp_supports_fec(intel_dp, pipe_config); > > > - > > > ret = intel_dp_compute_link_config(encoder, pipe_config, conn_state); > > > if (ret < 0) > > > return ret; > > > -- > > > 2.19.2 > > > > > > _______________________________________________ > > > Intel-gfx mailing list > > > Intel-gfx@lists.freedesktop.org > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Ville Syrjälä > Intel
On Tue, Mar 26, 2019 at 11:10:44AM -0700, Manasi Navare wrote: > On Tue, Mar 26, 2019 at 06:16:57PM +0200, Ville Syrjälä wrote: > > On Tue, Mar 26, 2019 at 09:00:27AM -0700, Manasi Navare wrote: > > > On Tue, Mar 26, 2019 at 04:49:02PM +0200, Ville Syrjala wrote: > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > > > > Currently we enable FEC even when DSC is no used. While that is > > > > theoretically valid supposedly there isn't much of a benefit from > > > > this. But more importantly we do not account for the FEC link > > > > bandwidth overhead (2.4%) in the non-DSC link bandwidth computations. > > > > So the code may think we have enough bandwidth when we in fact > > > > do not. > > > > > > > > Cc: Anusha Srivatsa <anusha.srivatsa@intel.com> > > > > Cc: Manasi Navare <manasi.d.navare@intel.comk> > > > > Fixes: 240999cf339f ("i915/dp/fec: Add fec_enable to the crtc state.") > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > --- > > > > drivers/gpu/drm/i915/intel_dp.c | 6 +++--- > > > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > > > > index 326de12c3f44..bbf678561509 100644 > > > > --- a/drivers/gpu/drm/i915/intel_dp.c > > > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > > > @@ -1925,6 +1925,9 @@ static int intel_dp_dsc_compute_config(struct intel_dp *intel_dp, > > > > int pipe_bpp; > > > > int ret; > > > > > > > > + pipe_config->fec_enable = !intel_dp_is_edp(intel_dp) && > > > > + intel_dp_supports_fec(intel_dp, pipe_config); > > > > + > > > > > > We could still not enable DSC after this point since it has more checks in this > > > function. Even though in that case we would fail the encoder config so wouldnt > > > matter if we have enabled FEC or not, but its less intutive. > > > IMHO, the ideal place to set the fec enable is in intel_dp_compute_link_config() > > > after the all to dsc_compute_config and set it only if pipe_config->dsc_params.compression_enable > > > > That would require changing intel_dp_supports_dsc() which I decided > > wasn't worth the hassle. > > Hmm, yea because intel_dp_supports_dsc depends on fec_enable. > In that case we would need to change that to use intel_dp_supports_fec(). TBH that makes more sense since > we should set dp_supports based on HW FEC capability and then set the crtc_state->fec_enable based > on crtc_state->dsc_params.compression_enable. But since it doesnt change the functionality, I am ok > either ways. So r-b?
On Tue, Mar 26, 2019 at 04:49:02PM +0200, Ville Syrjala wrote: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Currently we enable FEC even when DSC is no used. While that is > theoretically valid supposedly there isn't much of a benefit from > this. But more importantly we do not account for the FEC link > bandwidth overhead (2.4%) in the non-DSC link bandwidth computations. > So the code may think we have enough bandwidth when we in fact > do not. > > Cc: Anusha Srivatsa <anusha.srivatsa@intel.com> > Cc: Manasi Navare <manasi.d.navare@intel.comk> There is a typo in the email address: manasi.d.navare@intel.com > Fixes: 240999cf339f ("i915/dp/fec: Add fec_enable to the crtc state.") > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> Makes sense to me Reviewed-by: Manasi Navare <manasi.d.navare@intel.com> Manasi > --- > drivers/gpu/drm/i915/intel_dp.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index 326de12c3f44..bbf678561509 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -1925,6 +1925,9 @@ static int intel_dp_dsc_compute_config(struct intel_dp *intel_dp, > int pipe_bpp; > int ret; > > + pipe_config->fec_enable = !intel_dp_is_edp(intel_dp) && > + intel_dp_supports_fec(intel_dp, pipe_config); > + > if (!intel_dp_supports_dsc(intel_dp, pipe_config)) > return -EINVAL; > > @@ -2168,9 +2171,6 @@ intel_dp_compute_config(struct intel_encoder *encoder, > if (adjusted_mode->flags & DRM_MODE_FLAG_DBLCLK) > return -EINVAL; > > - pipe_config->fec_enable = !intel_dp_is_edp(intel_dp) && > - intel_dp_supports_fec(intel_dp, pipe_config); > - > ret = intel_dp_compute_link_config(encoder, pipe_config, conn_state); > if (ret < 0) > return ret; > -- > 2.19.2 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Thu, Apr 11, 2019 at 12:11:42PM -0700, Manasi Navare wrote: > On Tue, Mar 26, 2019 at 04:49:02PM +0200, Ville Syrjala wrote: > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > Currently we enable FEC even when DSC is no used. While that is > > theoretically valid supposedly there isn't much of a benefit from > > this. But more importantly we do not account for the FEC link > > bandwidth overhead (2.4%) in the non-DSC link bandwidth computations. > > So the code may think we have enough bandwidth when we in fact > > do not. > > > > Cc: Anusha Srivatsa <anusha.srivatsa@intel.com> > > Cc: Manasi Navare <manasi.d.navare@intel.comk> > > There is a typo in the email address: > manasi.d.navare@intel.com Thanks for the reminder. I already forgot about this and was about to push without fixing it. Sadly it seems I had copy pasted it from the same place to both of these patches, so one got pushed with the bad address. Hmm. Looks like this escaped into 5.0 already. Added cc:stable and pushed to dinq. Thanks for the review. > > > Fixes: 240999cf339f ("i915/dp/fec: Add fec_enable to the crtc state.") > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Makes sense to me > > Reviewed-by: Manasi Navare <manasi.d.navare@intel.com> > > Manasi > > > --- > > drivers/gpu/drm/i915/intel_dp.c | 6 +++--- > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > > index 326de12c3f44..bbf678561509 100644 > > --- a/drivers/gpu/drm/i915/intel_dp.c > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > @@ -1925,6 +1925,9 @@ static int intel_dp_dsc_compute_config(struct intel_dp *intel_dp, > > int pipe_bpp; > > int ret; > > > > + pipe_config->fec_enable = !intel_dp_is_edp(intel_dp) && > > + intel_dp_supports_fec(intel_dp, pipe_config); > > + > > if (!intel_dp_supports_dsc(intel_dp, pipe_config)) > > return -EINVAL; > > > > @@ -2168,9 +2171,6 @@ intel_dp_compute_config(struct intel_encoder *encoder, > > if (adjusted_mode->flags & DRM_MODE_FLAG_DBLCLK) > > return -EINVAL; > > > > - pipe_config->fec_enable = !intel_dp_is_edp(intel_dp) && > > - intel_dp_supports_fec(intel_dp, pipe_config); > > - > > ret = intel_dp_compute_link_config(encoder, pipe_config, conn_state); > > if (ret < 0) > > return ret; > > -- > > 2.19.2 > > > > _______________________________________________ > > 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_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 326de12c3f44..bbf678561509 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -1925,6 +1925,9 @@ static int intel_dp_dsc_compute_config(struct intel_dp *intel_dp, int pipe_bpp; int ret; + pipe_config->fec_enable = !intel_dp_is_edp(intel_dp) && + intel_dp_supports_fec(intel_dp, pipe_config); + if (!intel_dp_supports_dsc(intel_dp, pipe_config)) return -EINVAL; @@ -2168,9 +2171,6 @@ intel_dp_compute_config(struct intel_encoder *encoder, if (adjusted_mode->flags & DRM_MODE_FLAG_DBLCLK) return -EINVAL; - pipe_config->fec_enable = !intel_dp_is_edp(intel_dp) && - intel_dp_supports_fec(intel_dp, pipe_config); - ret = intel_dp_compute_link_config(encoder, pipe_config, conn_state); if (ret < 0) return ret;