[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