[Intel-gfx] [PATCH] drm/i915/dp: Tweak initial dpcd backlight.enabled value

Vivi, Rodrigo rodrigo.vivi at intel.com
Mon Oct 12 22:02:35 UTC 2020



> On Oct 12, 2020, at 2:47 PM, Lyude Paul <lyude at redhat.com> wrote:
> 
> Just pushed this, but it's not in drm-tip because it would seem that rebuilding
> drm-tip has failed, and the conflict doesn't appear to be from any of the
> patches I pushed so I'm getting the feeling from the DRM maintainer docs I
> should probably let one of the drm-misc-folks handle the conflict.

conflicts solved. feel free to push now.

For the drm-misc I simply went with the drm-misc-next solution and for the drm-intel
I went with the drm-intel-next-queued one.

> 
> On Mon, 2020-10-12 at 13:50 -0400, Sean Paul wrote:
>> On Tue, Sep 22, 2020 at 11:36 AM Lyude Paul <lyude at redhat.com> wrote:
>>> On Tue, 2020-09-22 at 09:39 -0400, Sean Paul wrote:
>>>> On Mon, Sep 21, 2020 at 6:35 PM Lyude Paul <lyude at redhat.com> wrote:
>>>>> So if I understand this correctly, it sounds like that some Pixelbooks
>>>>> boot up
>>>>> with DP_EDP_BACKLIGHT_BRIGHTNESS_MSB set to a non-zero value, without
>>>>> the
>>>>> panel actually having DPCD backlight controls enabled?
>>>> 
>>>> It boots with DP_EDP_BACKLIGHT_BRIGHTNESS_MSB == 0, which used to set
>>>> backlight.enabled = false. By changing backlight.level = max,
>>>> backlight.enabled is now set to true. This results in losing backlight
>>>> control on boot (since the enable routine is no longer invoked).
>>>> 
>>> Ahhh ok, I'm fine with that - review still stands :)
>> 
>> Pinging intel maintainers, could someone please apply this?
>> 
>> 
>> Sean
>> 
>>>> Sean
>>>> 
>>>>> If I'm understanding that correctly, then this patch looks good to me:
>>>>> 
>>>>> Reviewed-by: Lyude Paul <lyude at redhat.com>
>>>>> 
>>>>> On Thu, 2020-09-17 at 20:28 -0400, Sean Paul wrote:
>>>>>> From: Sean Paul <seanpaul at chromium.org>
>>>>>> 
>>>>>> In commit 79946723092b ("drm/i915: Assume 100% brightness when not in
>>>>>> DPCD control mode"), we fixed the brightness level when DPCD control
>>>>>> was
>>>>>> not active to max brightness. This is as good as we can guess since
>>>>>> most
>>>>>> backlights go on full when uncontrolled.
>>>>>> 
>>>>>> However in doing so we changed the semantics of the initial
>>>>>> 'backlight.enabled' value. At least on Pixelbooks, they  were relying
>>>>>> on the brightness level in DP_EDP_BACKLIGHT_BRIGHTNESS_MSB to be 0 on
>>>>>> boot such that enabled would be false. This causes the device to be
>>>>>> enabled when the brightness is set. Without this, brightness control
>>>>>> doesn't work. So by changing brightness to max, we also flipped
>>>>>> enabled
>>>>>> to be true on boot.
>>>>>> 
>>>>>> To fix this, make enabled a function of brightness and backlight
>>>>>> control
>>>>>> mechanism.
>>>>>> 
>>>>>> Fixes: 79946723092b ("drm/i915: Assume 100% brightness when not in
>>>>>> DPCD
>>>>>> control mode")
>>>>>> Cc: Lyude Paul <lyude at redhat.com>
>>>>>> Cc: Jani Nikula <jani.nikula at intel.com>
>>>>>> Cc: Juha-Pekka Heikkila <juhapekka.heikkila at gmail.com>
>>>>>> Cc: "Ville Syrjälä" <ville.syrjala at linux.intel.com>
>>>>>> Cc: Rodrigo Vivi <rodrigo.vivi at intel.com>
>>>>>> Cc: Kevin Chowski <chowski at chromium.org>>
>>>>>> Signed-off-by: Sean Paul <seanpaul at chromium.org>
>>>>>> ---
>>>>>> .../drm/i915/display/intel_dp_aux_backlight.c | 31 ++++++++++++----
>>>>>> ---
>>>>>> 1 file changed, 20 insertions(+), 11 deletions(-)
>>>>>> 
>>>>>> diff --git a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
>>>>>> b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
>>>>>> index acbd7eb66cbe..036f504ac7db 100644
>>>>>> --- a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
>>>>>> +++ b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
>>>>>> @@ -52,17 +52,11 @@ static void set_aux_backlight_enable(struct
>>>>>> intel_dp
>>>>>> *intel_dp, bool enable)
>>>>>>      }
>>>>>> }
>>>>>> 
>>>>>> -/*
>>>>>> - * Read the current backlight value from DPCD register(s) based
>>>>>> - * on if 8-bit(MSB) or 16-bit(MSB and LSB) values are supported
>>>>>> - */
>>>>>> -static u32 intel_dp_aux_get_backlight(struct intel_connector
>>>>>> *connector)
>>>>>> +static bool intel_dp_aux_backlight_dpcd_mode(struct intel_connector
>>>>>> *connector)
>>>>>> {
>>>>>>      struct intel_dp *intel_dp = intel_attached_dp(connector);
>>>>>>      struct drm_i915_private *i915 = dp_to_i915(intel_dp);
>>>>>> -     u8 read_val[2] = { 0x0 };
>>>>>>      u8 mode_reg;
>>>>>> -     u16 level = 0;
>>>>>> 
>>>>>>      if (drm_dp_dpcd_readb(&intel_dp->aux,
>>>>>>                            DP_EDP_BACKLIGHT_MODE_SET_REGISTER,
>>>>>> @@ -70,15 +64,29 @@ static u32 intel_dp_aux_get_backlight(struct
>>>>>> intel_connector *connector)
>>>>>>              drm_dbg_kms(&i915->drm,
>>>>>>                          "Failed to read the DPCD register 0x%x\n",
>>>>>>                          DP_EDP_BACKLIGHT_MODE_SET_REGISTER);
>>>>>> -             return 0;
>>>>>> +             return false;
>>>>>>      }
>>>>>> 
>>>>>> +     return (mode_reg & DP_EDP_BACKLIGHT_CONTROL_MODE_MASK) ==
>>>>>> +            DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD;
>>>>>> +}
>>>>>> +
>>>>>> +/*
>>>>>> + * Read the current backlight value from DPCD register(s) based
>>>>>> + * on if 8-bit(MSB) or 16-bit(MSB and LSB) values are supported
>>>>>> + */
>>>>>> +static u32 intel_dp_aux_get_backlight(struct intel_connector
>>>>>> *connector)
>>>>>> +{
>>>>>> +     struct intel_dp *intel_dp = intel_attached_dp(connector);
>>>>>> +     struct drm_i915_private *i915 = dp_to_i915(intel_dp);
>>>>>> +     u8 read_val[2] = { 0x0 };
>>>>>> +     u16 level = 0;
>>>>>> +
>>>>>>      /*
>>>>>>       * If we're not in DPCD control mode yet, the programmed
>>>>>> brightness
>>>>>>       * value is meaningless and we should assume max brightness
>>>>>>       */
>>>>>> -     if ((mode_reg & DP_EDP_BACKLIGHT_CONTROL_MODE_MASK) !=
>>>>>> -         DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD)
>>>>>> +     if (!intel_dp_aux_backlight_dpcd_mode(connector))
>>>>>>              return connector->panel.backlight.max;
>>>>>> 
>>>>>>      if (drm_dp_dpcd_read(&intel_dp->aux,
>>>>>> DP_EDP_BACKLIGHT_BRIGHTNESS_MSB,
>>>>>> @@ -319,7 +327,8 @@ static int intel_dp_aux_setup_backlight(struct
>>>>>> intel_connector *connector,
>>>>>> 
>>>>>>      panel->backlight.min = 0;
>>>>>>      panel->backlight.level = intel_dp_aux_get_backlight(connector);
>>>>>> -     panel->backlight.enabled = panel->backlight.level != 0;
>>>>>> +     panel->backlight.enabled =
>>>>>> intel_dp_aux_backlight_dpcd_mode(connector)
>>>>>> &&
>>>>>> +                                panel->backlight.level != 0;
>>>>>> 
>>>>>>      return 0;
>>>>>> }
>>>>> --
>>>>> Cheers,
>>>>>        Lyude Paul (she/her)
>>>>>        Software Engineer at Red Hat
>>>>> 
>>> --
>>> Cheers,
>>>        Lyude Paul (she/her)
>>>        Software Engineer at Red Hat
>>> 
> -- 
> Sincerely,
>      Lyude Paul (she/her)
>      Software Engineer at Red Hat
> 
> Note: I deal with a lot of emails and have a lot of bugs on my plate. If you've
> asked me a question, are waiting for a review/merge on a patch, etc. and I
> haven't responded in a while, please feel free to send me another email to check
> on my status. I don't bite!
> 



More information about the Intel-gfx mailing list