Re: [PATCH 1/5] drm/i915: Fix condition check for backlight control via DPCD

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

On Wed, 08 Mar 2017, Puthikorn Voravootivat <puthik@chromium.org> wrote:
> Currently the intel_dp_aux_backlight driver requires eDP panel
> to support these conditions to allow the backlight adjust via
> dpcd register.
> 1) DP_EDP_TCON_BACKLIGHT_ADJUSTMENT_CAP
> 2) DP_EDP_BACKLIGHT_AUX_ENABLE_CAP
> 3) not DP_EDP_BACKLIGHT_PIN_ENABLE_CAP
> 4) not DP_EDP_BACKLIGHT_BRIGHTNESS_PWM_PIN_CAP
>
> Condition 2,3,4 are incorect because of the following reasons.
> For 2), "backlight can be enabled via AUX" is not "brightness
> can be adjusted via AUX". For 3) and 4), this requires panel
> to not support backlight adjustment via eDP BL_PWM_DIM pin,
> but there are some panels that support both AUX and eDP pin.
>
> This patch fixes the condition by checking for these instead.
> 1) DP_EDP_TCON_BACKLIGHT_ADJUSTMENT_CAP
> 2) DP_EDP_BACKLIGHT_BRIGHTNESS_AUX_SET_CAP

I understand your point, but IIRC the goal with the current code was to
support DPCD backlight control *only* when PWM pin backlight control was
not possible. Your patch turns it around, and enables DPCD backlight
control whenever it's possible.

> 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.

It's also true that the current code conflates backlight enable and
adjustment caps. But that's largely because we also currently do not
support any type of mixed mode. And that would require quite a bit more
code. With your patch, you could end up with DPCD backlight adjustment
with no backligt enable/disable capability.

So I acknowledge the limitations in the current code, but I'm also not
convinced this patch series would be sufficient.


BR,
Jani.

>
> Signed-off-by: Puthikorn Voravootivat <puthik@chromium.org>
> ---
>  drivers/gpu/drm/i915/intel_dp_aux_backlight.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> index 6532e226db29..abfb09c1269f 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",
> @@ -142,10 +146,8 @@ intel_dp_aux_display_control_capable(struct intel_connector *connector)
>  	/* 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))) {
> +	if ((intel_dp->edp_dpcd[1] & DP_EDP_TCON_BACKLIGHT_ADJUSTMENT_CAP) &&
> +	    (intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_AUX_SET_CAP)) {
>  		DRM_DEBUG_KMS("AUX Backlight Control Supported!\n");
>  		return true;
>  	}

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