Re: [PATCH RESEND v4 1/6] drm/i915: Add DPCD preferred mode for backlight control

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

On Tue, 18 Apr 2017, Puthikorn Voravootivat <puthik@chromium.org> wrote:
> Currently the intel_dp_aux_backlight driver requires eDP panel
> to not also support backlight adjustment via PWM pin to use
> this driver.
>
> This force the eDP panel that support both ways of backlight
> adjustment to do it via PWM pin.

Currently this is by design. But I think we agreed previously that the
driver also has incorrect capability checks for the current design. The
first priority would be to fix those checks. And the patch order in the
series should reflect that.

> This patch adds the new prefer DPCD mode in the i915_param
> to make it enable to prefer DPCD over the PWM via kernel param.
>
> This patch also add a check to DP_EDP_BACKLIGHT_AUX_ENABLE_CAP
> in set_aux_backlight_enable() since the backlight enablement
> can be done via BL_ENABLE eDP connector pin in the case that
> it does not support doing that via AUX.

"also" in the commit message is a strong clue it should be a separate
patch. What you describe is potentially a fix that should precede this
patch.

>
> Signed-off-by: Puthikorn Voravootivat <puthik@chromium.org>
> ---
>  drivers/gpu/drm/i915/i915_params.c            |  6 ++---
>  drivers/gpu/drm/i915/i915_params.h            |  2 +-
>  drivers/gpu/drm/i915/intel_dp_aux_backlight.c | 33 +++++++++++++++++++--------
>  3 files changed, 27 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
> index b6a7e363d076..960393dd5edf 100644
> --- a/drivers/gpu/drm/i915/i915_params.c
> +++ b/drivers/gpu/drm/i915/i915_params.c
> @@ -63,7 +63,7 @@ struct i915_params i915 __read_mostly = {
>  	.huc_firmware_path = NULL,
>  	.enable_dp_mst = true,
>  	.inject_load_failure = 0,
> -	.enable_dpcd_backlight = false,
> +	.enable_dpcd_backlight = 0,
>  	.enable_gvt = false,
>  };
>  
> @@ -246,9 +246,9 @@ MODULE_PARM_DESC(enable_dp_mst,
>  module_param_named_unsafe(inject_load_failure, i915.inject_load_failure, uint, 0400);
>  MODULE_PARM_DESC(inject_load_failure,
>  	"Force an error after a number of failure check points (0:disabled (default), N:force failure at the Nth failure check point)");
> -module_param_named(enable_dpcd_backlight, i915.enable_dpcd_backlight, bool, 0600);
> +module_param_named(enable_dpcd_backlight, i915.enable_dpcd_backlight, int, 0600);
>  MODULE_PARM_DESC(enable_dpcd_backlight,
> -	"Enable support for DPCD backlight control (default:false)");
> +	"Enable support for DPCD backlight control (0:disable (default), 1:prefer PWM pin, 2: prefer DPCD)");

See my comments below. I think you need to rethink the options.

>  
>  module_param_named(enable_gvt, i915.enable_gvt, bool, 0400);
>  MODULE_PARM_DESC(enable_gvt,
> diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h
> index 34148cc8637c..bf6e2c60f697 100644
> --- a/drivers/gpu/drm/i915/i915_params.h
> +++ b/drivers/gpu/drm/i915/i915_params.h
> @@ -51,6 +51,7 @@
>  	func(int, use_mmio_flip); \
>  	func(int, mmio_debug); \
>  	func(int, edp_vswing); \
> +	func(int, enable_dpcd_backlight); \
>  	func(unsigned int, inject_load_failure); \
>  	/* leave bools at the end to not create holes */ \
>  	func(bool, alpha_support); \
> @@ -66,7 +67,6 @@
>  	func(bool, verbose_state_checks); \
>  	func(bool, nuclear_pageflip); \
>  	func(bool, enable_dp_mst); \
> -	func(bool, enable_dpcd_backlight); \
>  	func(bool, enable_gvt)
>  
>  #define MEMBER(T, member) T member
> diff --git a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> index 6532e226db29..42f73d9a3ccf 100644
> --- a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> +++ b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> @@ -28,6 +28,10 @@ static void set_aux_backlight_enable(struct intel_dp *intel_dp, bool enable)
>  {
>  	uint8_t reg_val = 0;
>  
> +	/* Early return when display use other mechanism to enable backlight. */
> +	if (!(intel_dp->edp_dpcd[1] & DP_EDP_BACKLIGHT_AUX_ENABLE_CAP))
> +		return;
> +
>  	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_EDP_DISPLAY_CONTROL_REGISTER,
>  			      &reg_val) < 0) {
>  		DRM_DEBUG_KMS("Failed to read DPCD register 0x%x\n",
> @@ -138,27 +142,36 @@ static bool
>  intel_dp_aux_display_control_capable(struct intel_connector *connector)
>  {
>  	struct intel_dp *intel_dp = enc_to_intel_dp(&connector->encoder->base);
> +	bool supported;
>  
>  	/* Check the  eDP Display control capabilities registers to determine if
>  	 * the panel can support backlight control over the aux channel
>  	 */
> -	if (intel_dp->edp_dpcd[1] & DP_EDP_TCON_BACKLIGHT_ADJUSTMENT_CAP &&
> -	    (intel_dp->edp_dpcd[1] & DP_EDP_BACKLIGHT_AUX_ENABLE_CAP) &&
> -	    !((intel_dp->edp_dpcd[1] & DP_EDP_BACKLIGHT_PIN_ENABLE_CAP) ||
> -	      (intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_PWM_PIN_CAP))) {
> -		DRM_DEBUG_KMS("AUX Backlight Control Supported!\n");
> -		return true;
> +	switch (i915.enable_dpcd_backlight) {
> +	case 1: /* prefer PWM pin */
> +		supported = (intel_dp->edp_dpcd[1] & DP_EDP_TCON_BACKLIGHT_ADJUSTMENT_CAP) &&
> +			    (intel_dp->edp_dpcd[1] & DP_EDP_BACKLIGHT_AUX_ENABLE_CAP) &&
> +			    !(intel_dp->edp_dpcd[1] & DP_EDP_BACKLIGHT_PIN_ENABLE_CAP) &&
> +			    !(intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_PWM_PIN_CAP);
> +		break;

This is not right. If we're going to use the PWM pin for backlight
control, it's what's in intel_panel.c, and
intel_dp_aux_init_backlight_funcs() should return -ENODEV. We only
support one or the other.

I think you probably need to have a hard look at Table 10-1 "Summary of
Backlight Control Modes Using DPCD Registers" in eDP 1.4b, and tell us
what modes you really want to support and how. For example, the product
mode or any DPCD/PWM mixed modes aren't very easily achieved with the
current design.

BR,
Jani.


> +	case 2: /* prefer DPCD */
> +		supported = (intel_dp->edp_dpcd[1] & DP_EDP_TCON_BACKLIGHT_ADJUSTMENT_CAP) &&
> +			    (intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_AUX_SET_CAP);
> +		break;
> +	default:
> +		supported = false;
>  	}
> -	return false;
> +
> +	if (supported)
> +		DRM_DEBUG_KMS("AUX Backlight Control Supported!\n");
> +
> +	return supported;
>  }
>  
>  int intel_dp_aux_init_backlight_funcs(struct intel_connector *intel_connector)
>  {
>  	struct intel_panel *panel = &intel_connector->panel;
>  
> -	if (!i915.enable_dpcd_backlight)
> -		return -ENODEV;
> -
>  	if (!intel_dp_aux_display_control_capable(intel_connector))
>  		return -ENODEV;

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx