On Tue, 22 Nov 2016, Manasi Navare <manasi.d.navare at intel.com> wrote:
> From: Jim Bride <jim.bride at linux.intel.com>
>
> For DP compliance we need to be able to control the output color
> type for the pipe associated with the DP port. When a specific DP
> compliance test is requested with specific BPC value, we read the BPC
> value from the DPCD register and store it in the intel_dp structure.
> If this BPC value in intel_dp structure has a non-zero value
> and we're on a display port connector, then we use the value to
> calculate the bpp for the pipe. For cases where we are
> not on DP or there has not been an overridden value then we behave
> as normal.
>
> Signed-off-by: Jim Bride <jim.bride at linux.intel.com>
> Signed-off-by: Manasi Navare <manasi.d.navare at intel.com>
> Cc: Jani Nikula <jani.nikula at linux.intel.com>
> Cc: Daniel Vetter <daniel.vetter at intel.com>
> Cc: Ville Syrjala <ville.syrjala at linux.intel.com>
> ---
> drivers/gpu/drm/i915/intel_display.c | 4 +++-
> drivers/gpu/drm/i915/intel_dp.c | 10 ++++++++++
> drivers/gpu/drm/i915/intel_dp_mst.c | 11 +++++++++--
> drivers/gpu/drm/i915/intel_drv.h | 1 +
> 4 files changed, 23 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index b7a7ed8..bb1cca2 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -12694,11 +12694,13 @@ static void
> intel_modeset_update_connector_atomic_state(struct drm_device *dev)
>
> /* Clamp display bpp to EDID value */
> for_each_connector_in_state(state, connector, connector_state, i) {
> +
Unrelated change, and one we don't want.
> if (connector_state->crtc != &crtc->base)
> continue;
>
> connected_sink_compute_bpp(to_intel_connector(connector),
> - pipe_config);
> + pipe_config);
> +
Unrelated change, and one we don't want.
> }
>
> return bpp;
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 6693918..f93e130 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1549,6 +1549,13 @@ static int intel_dp_compute_bpp(struct intel_dp
> *intel_dp,
> if (bpc > 0)
> bpp = min(bpp, 3*bpc);
>
> + /* For DP Compliance we override the computed bpp for the pipe */
> + if (intel_dp->compliance_force_bpc != 0) {
You don't actually *set* compliance_force_bpc in this patch, making all
of this a complicated nop. We don't want this kind of changes, because
if this regresses, it'll regress in the patch actually *using* the code,
i.e. the patch setting compliance_force_bpc, not this one.
The rationale is the same as for adding unused functions as prep
patches.
> + pipe_config->pipe_bpp = intel_dp->compliance_force_bpc*3;
How about making that compliance_force_bpp and setting it to * 3
directly in one place instead of sprinkling the * 3 all over the place?
> + DRM_DEBUG_KMS("Setting pipe_bpp to %d\n",
> + pipe_config->pipe_bpp);
> + }
> +
> return bpp;
> }
>
> @@ -1629,6 +1636,7 @@ static int intel_dp_compute_bpp(struct intel_dp
> *intel_dp,
> /* Walk through all bpp values. Luckily they're all nicely spaced with 2
> * bpc in between. */
> bpp = intel_dp_compute_bpp(intel_dp, pipe_config);
> +
Unrelated change. Please pay attention to these.
> if (is_edp(intel_dp)) {
>
> /* Get bpp from vbt only for panels that dont have bpp in edid
> */
> @@ -4109,6 +4117,7 @@ static void intel_dp_handle_test_request(struct
> intel_dp *intel_dp)
> intel_dp->compliance_test_data = 0;
> intel_dp->compliance_test_lane_count = 0;
> intel_dp->compliance_test_link_rate = 0;
> + intel_dp->compliance_force_bpc = 0;
Again, benefits from having a sub struct.
>
> /*
> * Now read the DPCD to see if it's actually running
> @@ -4434,6 +4443,7 @@ static bool intel_digital_port_connected(struct
> drm_i915_private *dev_priv,
> intel_dp->compliance_test_active = 0;
> intel_dp->compliance_test_type = 0;
> intel_dp->compliance_test_data = 0;
> + intel_dp->compliance_force_bpc = 0;
> intel_dp->compliance_test_lane_count = 0;
> intel_dp->compliance_test_link_rate = 0;
>
> diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c
> b/drivers/gpu/drm/i915/intel_dp_mst.c
> index b029d10..940f173 100644
> --- a/drivers/gpu/drm/i915/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> @@ -45,6 +45,14 @@ static bool intel_dp_mst_compute_config(struct
> intel_encoder *encoder,
>
> pipe_config->has_pch_encoder = false;
> bpp = 24;
> + /* For DP Compliance we need to ensurethat we can override
> + * the computed bpp for the pipe
> + */
Unnecessary comment.
> + if (intel_dp->compliance_force_bpc) {
> + bpp = intel_dp->compliance_force_bpc * 3;
> + DRM_DEBUG_KMS("Setting pipe bpp to %d\n",
> + bpp);
> + }
> /*
> * for MST we always configure max link bw - the spec doesn't
> * seem to suggest we should do otherwise.
> @@ -52,8 +60,7 @@ static bool intel_dp_mst_compute_config(struct
> intel_encoder *encoder,
> lane_count = drm_dp_max_lane_count(intel_dp->dpcd);
>
> pipe_config->lane_count = lane_count;
> -
> - pipe_config->pipe_bpp = 24;
> + pipe_config->pipe_bpp = bpp;
> pipe_config->port_clock = intel_dp_max_link_rate(intel_dp);
>
> state = pipe_config->base.state;
> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> index 1e88288..3eb428e 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -960,6 +960,7 @@ struct intel_dp {
> bool compliance_test_active;
> u8 compliance_test_lane_count;
> u8 compliance_test_link_rate;
> + unsigned long compliance_force_bpc; /* 0 for default or bpc to use */
unsigned long? How about plain int?
> };
>
> struct intel_lspcon {
--
Jani Nikula, Intel Open Source Technology Center
