| Message ID | 20190326142556.21176-6-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,1/6] drm/i915: Add broadcast RGB property for DP MST | expand |
On Tue, Mar 26, 2019 at 04:25:56PM +0200, Ville Syrjala wrote: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > 6bpc is only legal for RGB and RAW pixel encodings. For the rest > the minimum is 8bpc. Set our lower limit accordingly. > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> Pushed everything except this one. Thanks for the review. I actually meant to send this as a followup but it was on the same branch and so got included in the repost accidentally. > --- > drivers/gpu/drm/i915/intel_dp.c | 10 +++++++++- > drivers/gpu/drm/i915/intel_dp_mst.c | 2 +- > drivers/gpu/drm/i915/intel_drv.h | 1 + > 3 files changed, 11 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index 2aee526ed632..149fdfbcb343 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -2002,6 +2002,14 @@ static int intel_dp_dsc_compute_config(struct intel_dp *intel_dp, > return 0; > } > > +int intel_dp_min_bpp(const struct intel_crtc_state *crtc_state) > +{ > + if (crtc_state->output_format == INTEL_OUTPUT_FORMAT_RGB) > + return 6 * 3; > + else > + return 8 * 3; > +} > + > static int > intel_dp_compute_link_config(struct intel_encoder *encoder, > struct intel_crtc_state *pipe_config, > @@ -2025,7 +2033,7 @@ intel_dp_compute_link_config(struct intel_encoder *encoder, > limits.min_lane_count = 1; > limits.max_lane_count = intel_dp_max_lane_count(intel_dp); > > - limits.min_bpp = 6 * 3; > + limits.min_bpp = intel_dp_min_bpp(pipe_config); > limits.max_bpp = intel_dp_compute_bpp(intel_dp, pipe_config); > > if (intel_dp_is_edp(intel_dp) && intel_dp->edp_dpcd[0] < DP_EDP_14) { > diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c > index 6d2af7cf48e6..79c229184873 100644 > --- a/drivers/gpu/drm/i915/intel_dp_mst.c > +++ b/drivers/gpu/drm/i915/intel_dp_mst.c > @@ -119,7 +119,7 @@ static int intel_dp_mst_compute_config(struct intel_encoder *encoder, > limits.min_lane_count = > limits.max_lane_count = intel_dp_max_lane_count(intel_dp); > > - limits.min_bpp = 6 * 3; > + limits.min_bpp = intel_dp_min_bpp(pipe_config); > limits.max_bpp = pipe_config->pipe_bpp; > > intel_dp_adjust_compliance_config(intel_dp, pipe_config, &limits); > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index e79954c6271c..13f1b0367287 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -1919,6 +1919,7 @@ void intel_dp_adjust_compliance_config(struct intel_dp *intel_dp, > struct link_config_limits *limits); > bool intel_dp_limited_color_range(const struct intel_crtc_state *crtc_state, > const struct drm_connector_state *conn_state); > +int intel_dp_min_bpp(const struct intel_crtc_state *crtc_state); > bool intel_dp_port_enabled(struct drm_i915_private *dev_priv, > i915_reg_t dp_reg, enum port port, > enum pipe *pipe); > -- > 2.19.2
On Tue, 2019-03-26 at 16:25 +0200, Ville Syrjala wrote: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > 6bpc is only legal for RGB and RAW pixel encodings. For the rest > the minimum is 8bpc. Set our lower limit accordingly. Patch doesn't apply anymore, got a conflict in intel_drv.h. > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > drivers/gpu/drm/i915/intel_dp.c | 10 +++++++++- > drivers/gpu/drm/i915/intel_dp_mst.c | 2 +- > drivers/gpu/drm/i915/intel_drv.h | 1 + > 3 files changed, 11 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index 2aee526ed632..149fdfbcb343 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -2002,6 +2002,14 @@ static int intel_dp_dsc_compute_config(struct intel_dp > *intel_dp, > return 0; > } > > +int intel_dp_min_bpp(const struct intel_crtc_state *crtc_state) > +{ > + if (crtc_state->output_format == INTEL_OUTPUT_FORMAT_RGB) > + return 6 * 3; > + else > + return 8 * 3; Code matches spec, however I think there is a possibility of min_bpp becoming greater than max_bpp. The max_bpc property allows user space to set a value of 6 and limits.min_bpp can become 24 because of the code above. Add a check for that in compute_link_config()? Probably would mess up the compute_config() loop too. > +} > + > static int > intel_dp_compute_link_config(struct intel_encoder *encoder, > struct intel_crtc_state *pipe_config, > @@ -2025,7 +2033,7 @@ intel_dp_compute_link_config(struct intel_encoder > *encoder, > limits.min_lane_count = 1; > limits.max_lane_count = intel_dp_max_lane_count(intel_dp); > > - limits.min_bpp = 6 * 3; > + limits.min_bpp = intel_dp_min_bpp(pipe_config); > limits.max_bpp = intel_dp_compute_bpp(intel_dp, pipe_config); > > if (intel_dp_is_edp(intel_dp) && intel_dp->edp_dpcd[0] < DP_EDP_14) { > diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c > b/drivers/gpu/drm/i915/intel_dp_mst.c > index 6d2af7cf48e6..79c229184873 100644 > --- a/drivers/gpu/drm/i915/intel_dp_mst.c > +++ b/drivers/gpu/drm/i915/intel_dp_mst.c > @@ -119,7 +119,7 @@ static int intel_dp_mst_compute_config(struct > intel_encoder *encoder, > limits.min_lane_count = > limits.max_lane_count = intel_dp_max_lane_count(intel_dp); > > - limits.min_bpp = 6 * 3; > + limits.min_bpp = intel_dp_min_bpp(pipe_config); > limits.max_bpp = pipe_config->pipe_bpp; > > intel_dp_adjust_compliance_config(intel_dp, pipe_config, &limits); > diff --git a/drivers/gpu/drm/i915/intel_drv.h > b/drivers/gpu/drm/i915/intel_drv.h > index e79954c6271c..13f1b0367287 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -1919,6 +1919,7 @@ void intel_dp_adjust_compliance_config(struct intel_dp > *intel_dp, > struct link_config_limits *limits); > bool intel_dp_limited_color_range(const struct intel_crtc_state *crtc_state, > const struct drm_connector_state *conn_state); > +int intel_dp_min_bpp(const struct intel_crtc_state *crtc_state); > bool intel_dp_port_enabled(struct drm_i915_private *dev_priv, > i915_reg_t dp_reg, enum port port, > enum pipe *pipe);
On Tue, Apr 09, 2019 at 01:28:18PM -0700, Dhinakaran Pandiyan wrote: > On Tue, 2019-03-26 at 16:25 +0200, Ville Syrjala wrote: > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > 6bpc is only legal for RGB and RAW pixel encodings. For the rest > > the minimum is 8bpc. Set our lower limit accordingly. > > Patch doesn't apply anymore, got a conflict in intel_drv.h. > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > --- > > drivers/gpu/drm/i915/intel_dp.c | 10 +++++++++- > > drivers/gpu/drm/i915/intel_dp_mst.c | 2 +- > > drivers/gpu/drm/i915/intel_drv.h | 1 + > > 3 files changed, 11 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > > index 2aee526ed632..149fdfbcb343 100644 > > --- a/drivers/gpu/drm/i915/intel_dp.c > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > @@ -2002,6 +2002,14 @@ static int intel_dp_dsc_compute_config(struct intel_dp > > *intel_dp, > > return 0; > > } > > > > +int intel_dp_min_bpp(const struct intel_crtc_state *crtc_state) > > +{ > > + if (crtc_state->output_format == INTEL_OUTPUT_FORMAT_RGB) > > + return 6 * 3; > > + else > > + return 8 * 3; > Code matches spec, however I think there is a possibility of min_bpp becoming > greater than max_bpp. The max_bpc property allows user space to set a value of 6 > and limits.min_bpp can become 24 because of the code above. Add a check for that > in compute_link_config()? Probably would mess up the compute_config() loop too. The code looks correct. Ie. should just end up with -EINVAL. > > > > +} > > + > > static int > > intel_dp_compute_link_config(struct intel_encoder *encoder, > > struct intel_crtc_state *pipe_config, > > @@ -2025,7 +2033,7 @@ intel_dp_compute_link_config(struct intel_encoder > > *encoder, > > limits.min_lane_count = 1; > > limits.max_lane_count = intel_dp_max_lane_count(intel_dp); > > > > - limits.min_bpp = 6 * 3; > > + limits.min_bpp = intel_dp_min_bpp(pipe_config); > > limits.max_bpp = intel_dp_compute_bpp(intel_dp, pipe_config); > > > > if (intel_dp_is_edp(intel_dp) && intel_dp->edp_dpcd[0] < DP_EDP_14) { > > diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c > > b/drivers/gpu/drm/i915/intel_dp_mst.c > > index 6d2af7cf48e6..79c229184873 100644 > > --- a/drivers/gpu/drm/i915/intel_dp_mst.c > > +++ b/drivers/gpu/drm/i915/intel_dp_mst.c > > @@ -119,7 +119,7 @@ static int intel_dp_mst_compute_config(struct > > intel_encoder *encoder, > > limits.min_lane_count = > > limits.max_lane_count = intel_dp_max_lane_count(intel_dp); > > > > - limits.min_bpp = 6 * 3; > > + limits.min_bpp = intel_dp_min_bpp(pipe_config); > > limits.max_bpp = pipe_config->pipe_bpp; > > > > intel_dp_adjust_compliance_config(intel_dp, pipe_config, &limits); > > diff --git a/drivers/gpu/drm/i915/intel_drv.h > > b/drivers/gpu/drm/i915/intel_drv.h > > index e79954c6271c..13f1b0367287 100644 > > --- a/drivers/gpu/drm/i915/intel_drv.h > > +++ b/drivers/gpu/drm/i915/intel_drv.h > > @@ -1919,6 +1919,7 @@ void intel_dp_adjust_compliance_config(struct intel_dp > > *intel_dp, > > struct link_config_limits *limits); > > bool intel_dp_limited_color_range(const struct intel_crtc_state *crtc_state, > > const struct drm_connector_state *conn_state); > > +int intel_dp_min_bpp(const struct intel_crtc_state *crtc_state); > > bool intel_dp_port_enabled(struct drm_i915_private *dev_priv, > > i915_reg_t dp_reg, enum port port, > > enum pipe *pipe);
On Tue, 2019-04-09 at 23:38 +0300, Ville Syrjälä wrote: > On Tue, Apr 09, 2019 at 01:28:18PM -0700, Dhinakaran Pandiyan wrote: > > On Tue, 2019-03-26 at 16:25 +0200, Ville Syrjala wrote: > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > > 6bpc is only legal for RGB and RAW pixel encodings. For the rest > > > the minimum is 8bpc. Set our lower limit accordingly. > > > > Patch doesn't apply anymore, got a conflict in intel_drv.h. > > > > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > --- > > > drivers/gpu/drm/i915/intel_dp.c | 10 +++++++++- > > > drivers/gpu/drm/i915/intel_dp_mst.c | 2 +- > > > drivers/gpu/drm/i915/intel_drv.h | 1 + > > > 3 files changed, 11 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c > > > b/drivers/gpu/drm/i915/intel_dp.c > > > index 2aee526ed632..149fdfbcb343 100644 > > > --- a/drivers/gpu/drm/i915/intel_dp.c > > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > > @@ -2002,6 +2002,14 @@ static int intel_dp_dsc_compute_config(struct > > > intel_dp > > > *intel_dp, > > > return 0; > > > } > > > > > > +int intel_dp_min_bpp(const struct intel_crtc_state *crtc_state) > > > +{ > > > + if (crtc_state->output_format == INTEL_OUTPUT_FORMAT_RGB) > > > + return 6 * 3; > > > + else > > > + return 8 * 3; > > > > Code matches spec, however I think there is a possibility of min_bpp > > becoming > > greater than max_bpp. The max_bpc property allows user space to set a value > > of 6 > > and limits.min_bpp can become 24 because of the code above. Add a check for > > that > > in compute_link_config()? Probably would mess up the compute_config() loop > > too. > > The code looks correct. Ie. should just end up with -EINVAL. Yup, it does now as I read it carefully again :) Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> However, I don't like the fact we are hiding the detail that min_bpp can be > max_bpp. If someone decides add a link config optimization starting from min_bpp, it's easy to miss this detail. -DK > > > > > > > > +} > > > + > > > static int > > > intel_dp_compute_link_config(struct intel_encoder *encoder, > > > struct intel_crtc_state *pipe_config, > > > @@ -2025,7 +2033,7 @@ intel_dp_compute_link_config(struct intel_encoder > > > *encoder, > > > limits.min_lane_count = 1; > > > limits.max_lane_count = intel_dp_max_lane_count(intel_dp); > > > > > > - limits.min_bpp = 6 * 3; > > > + limits.min_bpp = intel_dp_min_bpp(pipe_config); > > > limits.max_bpp = intel_dp_compute_bpp(intel_dp, pipe_config); > > > > > > if (intel_dp_is_edp(intel_dp) && intel_dp->edp_dpcd[0] < DP_EDP_14) { > > > diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c > > > b/drivers/gpu/drm/i915/intel_dp_mst.c > > > index 6d2af7cf48e6..79c229184873 100644 > > > --- a/drivers/gpu/drm/i915/intel_dp_mst.c > > > +++ b/drivers/gpu/drm/i915/intel_dp_mst.c > > > @@ -119,7 +119,7 @@ static int intel_dp_mst_compute_config(struct > > > intel_encoder *encoder, > > > limits.min_lane_count = > > > limits.max_lane_count = intel_dp_max_lane_count(intel_dp); > > > > > > - limits.min_bpp = 6 * 3; > > > + limits.min_bpp = intel_dp_min_bpp(pipe_config); > > > limits.max_bpp = pipe_config->pipe_bpp; > > > > > > intel_dp_adjust_compliance_config(intel_dp, pipe_config, &limits); > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h > > > b/drivers/gpu/drm/i915/intel_drv.h > > > index e79954c6271c..13f1b0367287 100644 > > > --- a/drivers/gpu/drm/i915/intel_drv.h > > > +++ b/drivers/gpu/drm/i915/intel_drv.h > > > @@ -1919,6 +1919,7 @@ void intel_dp_adjust_compliance_config(struct > > > intel_dp > > > *intel_dp, > > > struct link_config_limits *limits); > > > bool intel_dp_limited_color_range(const struct intel_crtc_state > > > *crtc_state, > > > const struct drm_connector_state *conn_state); > > > +int intel_dp_min_bpp(const struct intel_crtc_state *crtc_state); > > > bool intel_dp_port_enabled(struct drm_i915_private *dev_priv, > > > i915_reg_t dp_reg, enum port port, > > > enum pipe *pipe); > >
On Tue, Apr 09, 2019 at 02:04:01PM -0700, Dhinakaran Pandiyan wrote: > On Tue, 2019-04-09 at 23:38 +0300, Ville Syrjälä wrote: > > On Tue, Apr 09, 2019 at 01:28:18PM -0700, Dhinakaran Pandiyan wrote: > > > On Tue, 2019-03-26 at 16:25 +0200, Ville Syrjala wrote: > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > > > > 6bpc is only legal for RGB and RAW pixel encodings. For the rest > > > > the minimum is 8bpc. Set our lower limit accordingly. > > > > > > Patch doesn't apply anymore, got a conflict in intel_drv.h. > > > > > > > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > --- > > > > drivers/gpu/drm/i915/intel_dp.c | 10 +++++++++- > > > > drivers/gpu/drm/i915/intel_dp_mst.c | 2 +- > > > > drivers/gpu/drm/i915/intel_drv.h | 1 + > > > > 3 files changed, 11 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c > > > > b/drivers/gpu/drm/i915/intel_dp.c > > > > index 2aee526ed632..149fdfbcb343 100644 > > > > --- a/drivers/gpu/drm/i915/intel_dp.c > > > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > > > @@ -2002,6 +2002,14 @@ static int intel_dp_dsc_compute_config(struct > > > > intel_dp > > > > *intel_dp, > > > > return 0; > > > > } > > > > > > > > +int intel_dp_min_bpp(const struct intel_crtc_state *crtc_state) > > > > +{ > > > > + if (crtc_state->output_format == INTEL_OUTPUT_FORMAT_RGB) > > > > + return 6 * 3; > > > > + else > > > > + return 8 * 3; > > > > > > Code matches spec, however I think there is a possibility of min_bpp > > > becoming > > > greater than max_bpp. The max_bpc property allows user space to set a value > > > of 6 > > > and limits.min_bpp can become 24 because of the code above. Add a check for > > > that > > > in compute_link_config()? Probably would mess up the compute_config() loop > > > too. > > > > The code looks correct. Ie. should just end up with -EINVAL. > Yup, it does now as I read it carefully again :) > Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> Ta. Pushed. > > However, I don't like the fact we are hiding the detail that min_bpp can be > > max_bpp. If someone decides add a link config optimization starting from > min_bpp, it's easy to miss this detail. I guess I wouldn't object to an explicit check for this. As a bonus we could add a more descriptive debug message for this case. > > -DK > > > > > > > > > > > > > +} > > > > + > > > > static int > > > > intel_dp_compute_link_config(struct intel_encoder *encoder, > > > > struct intel_crtc_state *pipe_config, > > > > @@ -2025,7 +2033,7 @@ intel_dp_compute_link_config(struct intel_encoder > > > > *encoder, > > > > limits.min_lane_count = 1; > > > > limits.max_lane_count = intel_dp_max_lane_count(intel_dp); > > > > > > > > - limits.min_bpp = 6 * 3; > > > > + limits.min_bpp = intel_dp_min_bpp(pipe_config); > > > > limits.max_bpp = intel_dp_compute_bpp(intel_dp, pipe_config); > > > > > > > > if (intel_dp_is_edp(intel_dp) && intel_dp->edp_dpcd[0] < DP_EDP_14) { > > > > diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c > > > > b/drivers/gpu/drm/i915/intel_dp_mst.c > > > > index 6d2af7cf48e6..79c229184873 100644 > > > > --- a/drivers/gpu/drm/i915/intel_dp_mst.c > > > > +++ b/drivers/gpu/drm/i915/intel_dp_mst.c > > > > @@ -119,7 +119,7 @@ static int intel_dp_mst_compute_config(struct > > > > intel_encoder *encoder, > > > > limits.min_lane_count = > > > > limits.max_lane_count = intel_dp_max_lane_count(intel_dp); > > > > > > > > - limits.min_bpp = 6 * 3; > > > > + limits.min_bpp = intel_dp_min_bpp(pipe_config); > > > > limits.max_bpp = pipe_config->pipe_bpp; > > > > > > > > intel_dp_adjust_compliance_config(intel_dp, pipe_config, &limits); > > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h > > > > b/drivers/gpu/drm/i915/intel_drv.h > > > > index e79954c6271c..13f1b0367287 100644 > > > > --- a/drivers/gpu/drm/i915/intel_drv.h > > > > +++ b/drivers/gpu/drm/i915/intel_drv.h > > > > @@ -1919,6 +1919,7 @@ void intel_dp_adjust_compliance_config(struct > > > > intel_dp > > > > *intel_dp, > > > > struct link_config_limits *limits); > > > > bool intel_dp_limited_color_range(const struct intel_crtc_state > > > > *crtc_state, > > > > const struct drm_connector_state *conn_state); > > > > +int intel_dp_min_bpp(const struct intel_crtc_state *crtc_state); > > > > bool intel_dp_port_enabled(struct drm_i915_private *dev_priv, > > > > i915_reg_t dp_reg, enum port port, > > > > enum pipe *pipe); > > > >
On Thu, 2019-04-11 at 21:27 +0300, Ville Syrjälä wrote: > On Tue, Apr 09, 2019 at 02:04:01PM -0700, Dhinakaran Pandiyan wrote: > > On Tue, 2019-04-09 at 23:38 +0300, Ville Syrjälä wrote: > > > On Tue, Apr 09, 2019 at 01:28:18PM -0700, Dhinakaran Pandiyan > > > wrote: > > > > On Tue, 2019-03-26 at 16:25 +0200, Ville Syrjala wrote: > > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > > > > > > 6bpc is only legal for RGB and RAW pixel encodings. For the > > > > > rest > > > > > the minimum is 8bpc. Set our lower limit accordingly. > > > > > > > > Patch doesn't apply anymore, got a conflict in intel_drv.h. > > > > > > > > > > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > --- > > > > > drivers/gpu/drm/i915/intel_dp.c | 10 +++++++++- > > > > > drivers/gpu/drm/i915/intel_dp_mst.c | 2 +- > > > > > drivers/gpu/drm/i915/intel_drv.h | 1 + > > > > > 3 files changed, 11 insertions(+), 2 deletions(-) > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c > > > > > b/drivers/gpu/drm/i915/intel_dp.c > > > > > index 2aee526ed632..149fdfbcb343 100644 > > > > > --- a/drivers/gpu/drm/i915/intel_dp.c > > > > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > > > > @@ -2002,6 +2002,14 @@ static int > > > > > intel_dp_dsc_compute_config(struct > > > > > intel_dp > > > > > *intel_dp, > > > > > return 0; > > > > > } > > > > > > > > > > +int intel_dp_min_bpp(const struct intel_crtc_state > > > > > *crtc_state) > > > > > +{ > > > > > + if (crtc_state->output_format == > > > > > INTEL_OUTPUT_FORMAT_RGB) > > > > > + return 6 * 3; > > > > > + else > > > > > + return 8 * 3; > > > > > > > > Code matches spec, however I think there is a possibility of > > > > min_bpp > > > > becoming > > > > greater than max_bpp. The max_bpc property allows user space to > > > > set a value > > > > of 6 > > > > and limits.min_bpp can become 24 because of the code above. Add > > > > a check for > > > > that > > > > in compute_link_config()? Probably would mess up the > > > > compute_config() loop > > > > too. > > > > > > The code looks correct. Ie. should just end up with -EINVAL. > > > > Yup, it does now as I read it carefully again :) > > Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > Ta. Pushed. Late on jumping the train but dont we have to limit the range exposed while attaching the "max bpc" as well in this case? - Radhakrishna(RK) Sripada > > > > > However, I don't like the fact we are hiding the detail that > > min_bpp can be > > > max_bpp. If someone decides add a link config optimization starting > > from > > min_bpp, it's easy to miss this detail. > > I guess I wouldn't object to an explicit check for this. As a bonus > we could add a more descriptive debug message for this case. > > > > > -DK > > > > > > > > > > > > > > > > > > +} > > > > > + > > > > > static int > > > > > intel_dp_compute_link_config(struct intel_encoder *encoder, > > > > > struct intel_crtc_state > > > > > *pipe_config, > > > > > @@ -2025,7 +2033,7 @@ intel_dp_compute_link_config(struct > > > > > intel_encoder > > > > > *encoder, > > > > > limits.min_lane_count = 1; > > > > > limits.max_lane_count = > > > > > intel_dp_max_lane_count(intel_dp); > > > > > > > > > > - limits.min_bpp = 6 * 3; > > > > > + limits.min_bpp = intel_dp_min_bpp(pipe_config); > > > > > limits.max_bpp = intel_dp_compute_bpp(intel_dp, > > > > > pipe_config); > > > > > > > > > > if (intel_dp_is_edp(intel_dp) && intel_dp->edp_dpcd[0] > > > > > < DP_EDP_14) { > > > > > diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c > > > > > b/drivers/gpu/drm/i915/intel_dp_mst.c > > > > > index 6d2af7cf48e6..79c229184873 100644 > > > > > --- a/drivers/gpu/drm/i915/intel_dp_mst.c > > > > > +++ b/drivers/gpu/drm/i915/intel_dp_mst.c > > > > > @@ -119,7 +119,7 @@ static int > > > > > intel_dp_mst_compute_config(struct > > > > > intel_encoder *encoder, > > > > > limits.min_lane_count = > > > > > limits.max_lane_count = > > > > > intel_dp_max_lane_count(intel_dp); > > > > > > > > > > - limits.min_bpp = 6 * 3; > > > > > + limits.min_bpp = intel_dp_min_bpp(pipe_config); > > > > > limits.max_bpp = pipe_config->pipe_bpp; > > > > > > > > > > intel_dp_adjust_compliance_config(intel_dp, > > > > > pipe_config, &limits); > > > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h > > > > > b/drivers/gpu/drm/i915/intel_drv.h > > > > > index e79954c6271c..13f1b0367287 100644 > > > > > --- a/drivers/gpu/drm/i915/intel_drv.h > > > > > +++ b/drivers/gpu/drm/i915/intel_drv.h > > > > > @@ -1919,6 +1919,7 @@ void > > > > > intel_dp_adjust_compliance_config(struct > > > > > intel_dp > > > > > *intel_dp, > > > > > struct > > > > > link_config_limits *limits); > > > > > bool intel_dp_limited_color_range(const struct > > > > > intel_crtc_state > > > > > *crtc_state, > > > > > const struct > > > > > drm_connector_state *conn_state); > > > > > +int intel_dp_min_bpp(const struct intel_crtc_state > > > > > *crtc_state); > > > > > bool intel_dp_port_enabled(struct drm_i915_private > > > > > *dev_priv, > > > > > i915_reg_t dp_reg, enum port port, > > > > > enum pipe *pipe); > > > > > > > >
On Thu, Apr 11, 2019 at 08:33:08PM +0000, Sripada, Radhakrishna wrote: > On Thu, 2019-04-11 at 21:27 +0300, Ville Syrjälä wrote: > > On Tue, Apr 09, 2019 at 02:04:01PM -0700, Dhinakaran Pandiyan wrote: > > > On Tue, 2019-04-09 at 23:38 +0300, Ville Syrjälä wrote: > > > > On Tue, Apr 09, 2019 at 01:28:18PM -0700, Dhinakaran Pandiyan > > > > wrote: > > > > > On Tue, 2019-03-26 at 16:25 +0200, Ville Syrjala wrote: > > > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > > > > > > > > 6bpc is only legal for RGB and RAW pixel encodings. For the > > > > > > rest > > > > > > the minimum is 8bpc. Set our lower limit accordingly. > > > > > > > > > > Patch doesn't apply anymore, got a conflict in intel_drv.h. > > > > > > > > > > > > > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > > --- > > > > > > drivers/gpu/drm/i915/intel_dp.c | 10 +++++++++- > > > > > > drivers/gpu/drm/i915/intel_dp_mst.c | 2 +- > > > > > > drivers/gpu/drm/i915/intel_drv.h | 1 + > > > > > > 3 files changed, 11 insertions(+), 2 deletions(-) > > > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c > > > > > > b/drivers/gpu/drm/i915/intel_dp.c > > > > > > index 2aee526ed632..149fdfbcb343 100644 > > > > > > --- a/drivers/gpu/drm/i915/intel_dp.c > > > > > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > > > > > @@ -2002,6 +2002,14 @@ static int > > > > > > intel_dp_dsc_compute_config(struct > > > > > > intel_dp > > > > > > *intel_dp, > > > > > > return 0; > > > > > > } > > > > > > > > > > > > +int intel_dp_min_bpp(const struct intel_crtc_state > > > > > > *crtc_state) > > > > > > +{ > > > > > > + if (crtc_state->output_format == > > > > > > INTEL_OUTPUT_FORMAT_RGB) > > > > > > + return 6 * 3; > > > > > > + else > > > > > > + return 8 * 3; > > > > > > > > > > Code matches spec, however I think there is a possibility of > > > > > min_bpp > > > > > becoming > > > > > greater than max_bpp. The max_bpc property allows user space to > > > > > set a value > > > > > of 6 > > > > > and limits.min_bpp can become 24 because of the code above. Add > > > > > a check for > > > > > that > > > > > in compute_link_config()? Probably would mess up the > > > > > compute_config() loop > > > > > too. > > > > > > > > The code looks correct. Ie. should just end up with -EINVAL. > > > > > > Yup, it does now as I read it carefully again :) > > > Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > > > Ta. Pushed. > Late on jumping the train but dont we have to limit the range exposed > while attaching the "max bpc" as well in this case? Late answering too. No we can't limit the range because we don't know ahead of time whether RGB or YCbCr is going to be used. Well, we could reject 6bpc entirely but that seems a bit silly too. The atomic check will simply fail if you try a combo that doesn't work.
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 2aee526ed632..149fdfbcb343 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -2002,6 +2002,14 @@ static int intel_dp_dsc_compute_config(struct intel_dp *intel_dp, return 0; } +int intel_dp_min_bpp(const struct intel_crtc_state *crtc_state) +{ + if (crtc_state->output_format == INTEL_OUTPUT_FORMAT_RGB) + return 6 * 3; + else + return 8 * 3; +} + static int intel_dp_compute_link_config(struct intel_encoder *encoder, struct intel_crtc_state *pipe_config, @@ -2025,7 +2033,7 @@ intel_dp_compute_link_config(struct intel_encoder *encoder, limits.min_lane_count = 1; limits.max_lane_count = intel_dp_max_lane_count(intel_dp); - limits.min_bpp = 6 * 3; + limits.min_bpp = intel_dp_min_bpp(pipe_config); limits.max_bpp = intel_dp_compute_bpp(intel_dp, pipe_config); if (intel_dp_is_edp(intel_dp) && intel_dp->edp_dpcd[0] < DP_EDP_14) { diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c index 6d2af7cf48e6..79c229184873 100644 --- a/drivers/gpu/drm/i915/intel_dp_mst.c +++ b/drivers/gpu/drm/i915/intel_dp_mst.c @@ -119,7 +119,7 @@ static int intel_dp_mst_compute_config(struct intel_encoder *encoder, limits.min_lane_count = limits.max_lane_count = intel_dp_max_lane_count(intel_dp); - limits.min_bpp = 6 * 3; + limits.min_bpp = intel_dp_min_bpp(pipe_config); limits.max_bpp = pipe_config->pipe_bpp; intel_dp_adjust_compliance_config(intel_dp, pipe_config, &limits); diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index e79954c6271c..13f1b0367287 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -1919,6 +1919,7 @@ void intel_dp_adjust_compliance_config(struct intel_dp *intel_dp, struct link_config_limits *limits); bool intel_dp_limited_color_range(const struct intel_crtc_state *crtc_state, const struct drm_connector_state *conn_state); +int intel_dp_min_bpp(const struct intel_crtc_state *crtc_state); bool intel_dp_port_enabled(struct drm_i915_private *dev_priv, i915_reg_t dp_reg, enum port port, enum pipe *pipe);