| Message ID | 20181011001252.19370-2-radhakrishna.sripada@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v12,1/2] drm: Add connector property to limit max bpc | expand |
On Wed, 2018-10-10 at 17:12 -0700, Radhakrishna Sripada wrote: > Use the newly added "max bpc" connector property to limit pipe bpp. > > V3: Use drm_connector_state to access the "max bpc" property > V4: Initialize the drm property, add suuport to DP(Ville) > V5: Use the property in the connector and fix CI failure(Ville) > V6: Use the core function to attach max_bpc property, remove the > redundant > clamping of pipe bpp based on connector info > V7: Fix Checkpatch warnings > V9: Cleanup connected_sink_max_bpp and fix initial value in DP(Ville) > V12: Fix debug message(Ville) > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > Cc: Kishore Kadiyala <kishore.kadiyala@intel.com> > Cc: Manasi Navare <manasi.d.navare@intel.com> > Cc: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > Signed-off-by: Radhakrishna Sripada <radhakrishna.sripada@intel.com> > --- > drivers/gpu/drm/i915/intel_display.c | 48 +++++++++++++++++++++----- > ---------- > drivers/gpu/drm/i915/intel_dp.c | 4 +++ > drivers/gpu/drm/i915/intel_hdmi.c | 5 ++++ > 3 files changed, 37 insertions(+), 20 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c > b/drivers/gpu/drm/i915/intel_display.c > index a145efba9157..a597c4410f5d 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -10869,30 +10869,38 @@ static void > intel_modeset_update_connector_atomic_state(struct drm_device *dev) > drm_connector_list_iter_end(&conn_iter); > } > > -static void > -connected_sink_compute_bpp(struct intel_connector *connector, > - struct intel_crtc_state *pipe_config) > +static int > +connected_sink_max_bpp(const struct drm_connector_state *conn_state, > + struct intel_crtc_state *pipe_config) > { > - const struct drm_display_info *info = &connector- > >base.display_info; > - int bpp = pipe_config->pipe_bpp; > + int bpp; > + struct drm_display_info *info = &conn_state->connector- > >display_info; > > - DRM_DEBUG_KMS("[CONNECTOR:%d:%s] checking for sink bpp > constrains\n", > - connector->base.base.id, > - connector->base.name); > + bpp = min(pipe_config->pipe_bpp, conn_state->max_bpc * 3); > > - /* Don't use an invalid EDID bpc value */ > - if (info->bpc != 0 && info->bpc * 3 < bpp) { > - DRM_DEBUG_KMS("clamping display bpp (was %d) to EDID > reported max of %d\n", > - bpp, info->bpc * 3); > - pipe_config->pipe_bpp = info->bpc * 3; > + switch (conn_state->max_bpc) { > + case 6 ... 7: > + pipe_config->pipe_bpp = 6 * 3; > + case 8 ... 9: > + pipe_config->pipe_bpp = 8 * 3; > + break; > + case 10 ... 11: > + pipe_config->pipe_bpp = 10 * 3; > + break; > + case 12: > + pipe_config->pipe_bpp = 12 * 3; > + break; > + default: > + return -EINVAL; > } Why do we need this assignment here? I mean you have already determined the minimum value which is stored in bpp, which should be used and then check again, that it is not equal and only then use it. Could it be just as simple as this(or I simply misunderstand something here): switch (conn_state->max_bpc) { case 6 ... 7: bpp = 6 * 3; ... default: return -EINVAL; } so here we set bpp(instead of pipe_bpp) to correct value or return EINVAL. And then we simply assign pipe_bpp to minimum of those and return: pipe_bpp = min(pipe_config->pipe_bpp, bpp); return 0; Also that way get bpp values also check to correspond to the ranges given in cases labels, while initial expression min(pipe_config->pipe_bpp, conn_state->max_bpc * 3) could possibly give 7 * 3 but not 6 * 3(as specified in "case 6 .. 7") for conn_state->max_bpc = 7. So then there is no need in next additional assignment and check: > > - /* Clamp bpp to 8 on screens without EDID 1.4 */ > - if (info->bpc == 0 && bpp > 24) { > - DRM_DEBUG_KMS("clamping display bpp (was %d) to > default limit of 24\n", > - bpp); > - pipe_config->pipe_bpp = 24; > + if (bpp != pipe_config->pipe_bpp) { > + DRM_DEBUG_KMS("Limiting display bpp to %d instead of > Edid bpp " > + "%d, requested bpp %d\n", bpp, 3 * > info->bpc, > + 3 * conn_state->max_requested_bpc); > + pipe_config->pipe_bpp = bpp; > } > + return 0; > } > > static int > @@ -10923,8 +10931,8 @@ compute_baseline_pipe_bpp(struct intel_crtc > *crtc, > if (connector_state->crtc != &crtc->base) > continue; > > - connected_sink_compute_bpp(to_intel_connector(connec > tor), > - pipe_config); > + if (connected_sink_max_bpp(connector_state, > pipe_config) < 0) > + return -EINVAL; > } > > return bpp; > diff --git a/drivers/gpu/drm/i915/intel_dp.c > b/drivers/gpu/drm/i915/intel_dp.c > index 0855b9785f7b..863c8832fe8b 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -5705,6 +5705,10 @@ intel_dp_add_properties(struct intel_dp > *intel_dp, struct drm_connector *connect > intel_attach_force_audio_property(connector); > > intel_attach_broadcast_rgb_property(connector); > + if (HAS_GMCH_DISPLAY(dev_priv)) > + drm_connector_attach_max_bpc_property(connector, 6, > 10); > + else if (INTEL_GEN(dev_priv) >= 5) > + drm_connector_attach_max_bpc_property(connector, 6, > 12); > > if (intel_dp_is_edp(intel_dp)) { > u32 allowed_scalers; > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c > b/drivers/gpu/drm/i915/intel_hdmi.c > index 2c53efc463e6..3158ab085a30 100644 > --- a/drivers/gpu/drm/i915/intel_hdmi.c > +++ b/drivers/gpu/drm/i915/intel_hdmi.c > @@ -2103,11 +2103,16 @@ static const struct drm_encoder_funcs > intel_hdmi_enc_funcs = { > static void > intel_hdmi_add_properties(struct intel_hdmi *intel_hdmi, struct > drm_connector *connector) > { > + struct drm_i915_private *dev_priv = to_i915(connector->dev); > + > intel_attach_force_audio_property(connector); > intel_attach_broadcast_rgb_property(connector); > intel_attach_aspect_ratio_property(connector); > drm_connector_attach_content_type_property(connector); > connector->state->picture_aspect_ratio = > HDMI_PICTURE_ASPECT_NONE; > + > + if (!HAS_GMCH_DISPLAY(dev_priv)) > + drm_connector_attach_max_bpc_property(connector, 8, > 12); > } > > /*
On Thu, Oct 11, 2018 at 12:55:15AM -0700, Lisovskiy, Stanislav wrote: > On Wed, 2018-10-10 at 17:12 -0700, Radhakrishna Sripada wrote: > > Use the newly added "max bpc" connector property to limit pipe bpp. > > > > V3: Use drm_connector_state to access the "max bpc" property > > V4: Initialize the drm property, add suuport to DP(Ville) > > V5: Use the property in the connector and fix CI failure(Ville) > > V6: Use the core function to attach max_bpc property, remove the > > redundant > > clamping of pipe bpp based on connector info > > V7: Fix Checkpatch warnings > > V9: Cleanup connected_sink_max_bpp and fix initial value in DP(Ville) > > V12: Fix debug message(Ville) > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > Cc: Kishore Kadiyala <kishore.kadiyala@intel.com> > > Cc: Manasi Navare <manasi.d.navare@intel.com> > > Cc: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > > Signed-off-by: Radhakrishna Sripada <radhakrishna.sripada@intel.com> > > --- > > drivers/gpu/drm/i915/intel_display.c | 48 +++++++++++++++++++++----- > > ---------- > > drivers/gpu/drm/i915/intel_dp.c | 4 +++ > > drivers/gpu/drm/i915/intel_hdmi.c | 5 ++++ > > 3 files changed, 37 insertions(+), 20 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c > > b/drivers/gpu/drm/i915/intel_display.c > > index a145efba9157..a597c4410f5d 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -10869,30 +10869,38 @@ static void > > intel_modeset_update_connector_atomic_state(struct drm_device *dev) > > drm_connector_list_iter_end(&conn_iter); > > } > > > > -static void > > -connected_sink_compute_bpp(struct intel_connector *connector, > > - struct intel_crtc_state *pipe_config) > > +static int > > +connected_sink_max_bpp(const struct drm_connector_state *conn_state, > > + struct intel_crtc_state *pipe_config) > > { > > - const struct drm_display_info *info = &connector- > > >base.display_info; > > - int bpp = pipe_config->pipe_bpp; > > + int bpp; > > + struct drm_display_info *info = &conn_state->connector- > > >display_info; > > > > - DRM_DEBUG_KMS("[CONNECTOR:%d:%s] checking for sink bpp > > constrains\n", > > - connector->base.base.id, > > - connector->base.name); > > + bpp = min(pipe_config->pipe_bpp, conn_state->max_bpc * 3); > > > > - /* Don't use an invalid EDID bpc value */ > > - if (info->bpc != 0 && info->bpc * 3 < bpp) { > > - DRM_DEBUG_KMS("clamping display bpp (was %d) to EDID > > reported max of %d\n", > > - bpp, info->bpc * 3); > > - pipe_config->pipe_bpp = info->bpc * 3; > > + switch (conn_state->max_bpc) { > > + case 6 ... 7: > > + pipe_config->pipe_bpp = 6 * 3; > > + case 8 ... 9: > > + pipe_config->pipe_bpp = 8 * 3; > > + break; > > + case 10 ... 11: > > + pipe_config->pipe_bpp = 10 * 3; > > + break; > > + case 12: > > + pipe_config->pipe_bpp = 12 * 3; > > + break; > > + default: > > + return -EINVAL; > > } > > Why do we need this assignment here? I mean you have already determined > the minimum value which is stored in bpp, which should be used and then > check again, that it is not equal and only then use it. > Could it be just as simple as this(or I simply misunderstand something > here): > > switch (conn_state->max_bpc) { > case 6 ... 7: > bpp = 6 * 3; > ... > default: > return -EINVAL; > } > > so here we set bpp(instead of pipe_bpp) to correct value or return > EINVAL. > And then we simply assign pipe_bpp to minimum of those and return: > > pipe_bpp = min(pipe_config->pipe_bpp, bpp); > > return 0; > > Also that way get bpp values also check to correspond to the ranges > given in cases labels, while initial expression > min(pipe_config->pipe_bpp, conn_state->max_bpc * 3) could possibly give > 7 * 3 but not 6 * 3(as specified in "case 6 .. 7") for > conn_state->max_bpc = 7. > > So then there is no need in next additional assignment and check: This actually simplifies. I was trying to modify already existing code. This definitely looks cleaner. Will try these changes in the next rev. Thanks, Radhakrishna(RK) Sripada > > > > > - /* Clamp bpp to 8 on screens without EDID 1.4 */ > > - if (info->bpc == 0 && bpp > 24) { > > - DRM_DEBUG_KMS("clamping display bpp (was %d) to > > default limit of 24\n", > > - bpp); > > - pipe_config->pipe_bpp = 24; > > + if (bpp != pipe_config->pipe_bpp) { > > + DRM_DEBUG_KMS("Limiting display bpp to %d instead of > > Edid bpp " > > + "%d, requested bpp %d\n", bpp, 3 * > > info->bpc, > > + 3 * conn_state->max_requested_bpc); > > + pipe_config->pipe_bpp = bpp; > > } > > + return 0; > > } > > > > static int > > @@ -10923,8 +10931,8 @@ compute_baseline_pipe_bpp(struct intel_crtc > > *crtc, > > if (connector_state->crtc != &crtc->base) > > continue; > > > > - connected_sink_compute_bpp(to_intel_connector(connec > > tor), > > - pipe_config); > > + if (connected_sink_max_bpp(connector_state, > > pipe_config) < 0) > > + return -EINVAL; > > } > > > > return bpp; > > diff --git a/drivers/gpu/drm/i915/intel_dp.c > > b/drivers/gpu/drm/i915/intel_dp.c > > index 0855b9785f7b..863c8832fe8b 100644 > > --- a/drivers/gpu/drm/i915/intel_dp.c > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > @@ -5705,6 +5705,10 @@ intel_dp_add_properties(struct intel_dp > > *intel_dp, struct drm_connector *connect > > intel_attach_force_audio_property(connector); > > > > intel_attach_broadcast_rgb_property(connector); > > + if (HAS_GMCH_DISPLAY(dev_priv)) > > + drm_connector_attach_max_bpc_property(connector, 6, > > 10); > > + else if (INTEL_GEN(dev_priv) >= 5) > > + drm_connector_attach_max_bpc_property(connector, 6, > > 12); > > > > if (intel_dp_is_edp(intel_dp)) { > > u32 allowed_scalers; > > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c > > b/drivers/gpu/drm/i915/intel_hdmi.c > > index 2c53efc463e6..3158ab085a30 100644 > > --- a/drivers/gpu/drm/i915/intel_hdmi.c > > +++ b/drivers/gpu/drm/i915/intel_hdmi.c > > @@ -2103,11 +2103,16 @@ static const struct drm_encoder_funcs > > intel_hdmi_enc_funcs = { > > static void > > intel_hdmi_add_properties(struct intel_hdmi *intel_hdmi, struct > > drm_connector *connector) > > { > > + struct drm_i915_private *dev_priv = to_i915(connector->dev); > > + > > intel_attach_force_audio_property(connector); > > intel_attach_broadcast_rgb_property(connector); > > intel_attach_aspect_ratio_property(connector); > > drm_connector_attach_content_type_property(connector); > > connector->state->picture_aspect_ratio = > > HDMI_PICTURE_ASPECT_NONE; > > + > > + if (!HAS_GMCH_DISPLAY(dev_priv)) > > + drm_connector_attach_max_bpc_property(connector, 8, > > 12); > > } > > > > /* > -- > Best Regards, > > Lisovskiy Stanislav
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index a145efba9157..a597c4410f5d 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -10869,30 +10869,38 @@ static void intel_modeset_update_connector_atomic_state(struct drm_device *dev) drm_connector_list_iter_end(&conn_iter); } -static void -connected_sink_compute_bpp(struct intel_connector *connector, - struct intel_crtc_state *pipe_config) +static int +connected_sink_max_bpp(const struct drm_connector_state *conn_state, + struct intel_crtc_state *pipe_config) { - const struct drm_display_info *info = &connector->base.display_info; - int bpp = pipe_config->pipe_bpp; + int bpp; + struct drm_display_info *info = &conn_state->connector->display_info; - DRM_DEBUG_KMS("[CONNECTOR:%d:%s] checking for sink bpp constrains\n", - connector->base.base.id, - connector->base.name); + bpp = min(pipe_config->pipe_bpp, conn_state->max_bpc * 3); - /* Don't use an invalid EDID bpc value */ - if (info->bpc != 0 && info->bpc * 3 < bpp) { - DRM_DEBUG_KMS("clamping display bpp (was %d) to EDID reported max of %d\n", - bpp, info->bpc * 3); - pipe_config->pipe_bpp = info->bpc * 3; + switch (conn_state->max_bpc) { + case 6 ... 7: + pipe_config->pipe_bpp = 6 * 3; + case 8 ... 9: + pipe_config->pipe_bpp = 8 * 3; + break; + case 10 ... 11: + pipe_config->pipe_bpp = 10 * 3; + break; + case 12: + pipe_config->pipe_bpp = 12 * 3; + break; + default: + return -EINVAL; } - /* Clamp bpp to 8 on screens without EDID 1.4 */ - if (info->bpc == 0 && bpp > 24) { - DRM_DEBUG_KMS("clamping display bpp (was %d) to default limit of 24\n", - bpp); - pipe_config->pipe_bpp = 24; + if (bpp != pipe_config->pipe_bpp) { + DRM_DEBUG_KMS("Limiting display bpp to %d instead of Edid bpp " + "%d, requested bpp %d\n", bpp, 3 * info->bpc, + 3 * conn_state->max_requested_bpc); + pipe_config->pipe_bpp = bpp; } + return 0; } static int @@ -10923,8 +10931,8 @@ compute_baseline_pipe_bpp(struct intel_crtc *crtc, if (connector_state->crtc != &crtc->base) continue; - connected_sink_compute_bpp(to_intel_connector(connector), - pipe_config); + if (connected_sink_max_bpp(connector_state, pipe_config) < 0) + return -EINVAL; } return bpp; diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 0855b9785f7b..863c8832fe8b 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -5705,6 +5705,10 @@ intel_dp_add_properties(struct intel_dp *intel_dp, struct drm_connector *connect intel_attach_force_audio_property(connector); intel_attach_broadcast_rgb_property(connector); + if (HAS_GMCH_DISPLAY(dev_priv)) + drm_connector_attach_max_bpc_property(connector, 6, 10); + else if (INTEL_GEN(dev_priv) >= 5) + drm_connector_attach_max_bpc_property(connector, 6, 12); if (intel_dp_is_edp(intel_dp)) { u32 allowed_scalers; diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c index 2c53efc463e6..3158ab085a30 100644 --- a/drivers/gpu/drm/i915/intel_hdmi.c +++ b/drivers/gpu/drm/i915/intel_hdmi.c @@ -2103,11 +2103,16 @@ static const struct drm_encoder_funcs intel_hdmi_enc_funcs = { static void intel_hdmi_add_properties(struct intel_hdmi *intel_hdmi, struct drm_connector *connector) { + struct drm_i915_private *dev_priv = to_i915(connector->dev); + intel_attach_force_audio_property(connector); intel_attach_broadcast_rgb_property(connector); intel_attach_aspect_ratio_property(connector); drm_connector_attach_content_type_property(connector); connector->state->picture_aspect_ratio = HDMI_PICTURE_ASPECT_NONE; + + if (!HAS_GMCH_DISPLAY(dev_priv)) + drm_connector_attach_max_bpc_property(connector, 8, 12); } /*
Use the newly added "max bpc" connector property to limit pipe bpp. V3: Use drm_connector_state to access the "max bpc" property V4: Initialize the drm property, add suuport to DP(Ville) V5: Use the property in the connector and fix CI failure(Ville) V6: Use the core function to attach max_bpc property, remove the redundant clamping of pipe bpp based on connector info V7: Fix Checkpatch warnings V9: Cleanup connected_sink_max_bpp and fix initial value in DP(Ville) V12: Fix debug message(Ville) Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> Cc: Kishore Kadiyala <kishore.kadiyala@intel.com> Cc: Manasi Navare <manasi.d.navare@intel.com> Cc: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> Signed-off-by: Radhakrishna Sripada <radhakrishna.sripada@intel.com> --- drivers/gpu/drm/i915/intel_display.c | 48 +++++++++++++++++++++--------------- drivers/gpu/drm/i915/intel_dp.c | 4 +++ drivers/gpu/drm/i915/intel_hdmi.c | 5 ++++ 3 files changed, 37 insertions(+), 20 deletions(-)