| Message ID | 20170308213053.194062-2-puthik@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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, > ®_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; > }
I think that there won't be a good way to determine which way of
adjusting backlight
is preferred when the panel support both PWM pin and DPCD.
How about extending the current i915_params.enable_dpcd_backlight to this.
{ 0:disable(default) 1: prefer PWM pin, 2: prefer DPCD }
If it is 1), use the existing condition and if it is 2) use my new condition.
This would solve the limitation of the current code and avoid the
mixed mode complexity
since user need to purposely set the param to 2 to prefer DPCD.
Thanks
On Thu, Mar 9, 2017 at 1:50 AM, Jani Nikula <jani.nikula@linux.intel.com> wrote:
> 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,
>> ®_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
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, ®_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; }
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 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. 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(-)