Re: [PATCH 3/5] Add support for forcing 6 bpc on DP pipes.

[Jani Nikula <jani.nikula@linux.intel.com>]

On Tue, 22 Nov 2016, Manasi Navare <manasi.d.navare@intel.com> wrote:
> From: Jim Bride <jim.bride@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@linux.intel.com>
> Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Cc: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Ville Syrjala <ville.syrjala@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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx