From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id A69D9C4332F for ; Tue, 28 Sep 2021 20:00:21 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 89D94610E6 for ; Tue, 28 Sep 2021 20:00:21 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S242602AbhI1UCA (ORCPT ); Tue, 28 Sep 2021 16:02:00 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:45014 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S242535AbhI1UB6 (ORCPT ); Tue, 28 Sep 2021 16:01:58 -0400 Received: from mail-io1-xd33.google.com (mail-io1-xd33.google.com [IPv6:2607:f8b0:4864:20::d33]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 90069C06161C for ; Tue, 28 Sep 2021 13:00:18 -0700 (PDT) Received: by mail-io1-xd33.google.com with SMTP id p80so104502iod.10 for ; Tue, 28 Sep 2021 13:00:18 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=tt9tt/F3IxLyQ34cK+9pj6tNPzMMJ1v10W8CXUTZIXc=; b=N3sK4GpDM7NoausvCZ7uvCv2xW8y1o2GqpLxTtHdEz+qukAhK9GRVXI15tRB36rXkG nIIXXm2zgad8mAZpCf5s5YQsjUW2aE4PBDSL5ia/k1Nt9Mo4BFgPQLAOAlIwoYQXGXuH fmr97KMI5Q8zlbFdiIq/dYgMzPBdrUuYP8EE4= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=tt9tt/F3IxLyQ34cK+9pj6tNPzMMJ1v10W8CXUTZIXc=; b=EUMCHqMSOPukuIYrexg8rgzda8ik8uOK2S8e/Xe6j4KnyTVyeLqqTzY1+v7Ge56ABf vOOm3G46LBdRoY+ZP/9sVdCUjXnLnvScrlNqpiflZ6iSYfLrQyDOfSHHbCDzbKwZxLEc pt4tI3ucJSB+Zrln+wnrwmv7xsmQmynD319MC0wtWYmDW3CU2Ipbt6GtdfjKJ7Vn1t39 upYAh9AHZGtfiY5m9JxiEpN5rLPM/TQHK0ccSo1jTKe1iBDh1gVSL9GyU1QFSJQSluFj chhs1Uc5oLGSoGKm3fZnHxZ12R+5XuqOA8HmjgLKXLZDq38a/pDT1OcE6Lj9LSsYRMIl 0k2w== X-Gm-Message-State: AOAM530VOV8DIxLigqsszIhQBgk2QBzQMiq3acZ/HM1xDUTS3j7s1NuO DWsZPINC+lMVYhrKiq5xDvUp/ME7Q7+ydA== X-Google-Smtp-Source: ABdhPJz46h5/SAf+ZB5e/6ZuYjDCAcSsf6UnmGCXQxrZiw4d6he07xc3cNEpGomGLTyzfeHR9UZOtQ== X-Received: by 2002:a05:6638:2183:: with SMTP id s3mr6231605jaj.11.1632859217392; Tue, 28 Sep 2021 13:00:17 -0700 (PDT) Received: from mail-il1-f176.google.com (mail-il1-f176.google.com. [209.85.166.176]) by smtp.gmail.com with ESMTPSA id r7sm11392215ilm.5.2021.09.28.13.00.15 for (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 28 Sep 2021 13:00:16 -0700 (PDT) Received: by mail-il1-f176.google.com with SMTP id y15so239446ilu.12 for ; Tue, 28 Sep 2021 13:00:15 -0700 (PDT) X-Received: by 2002:a05:6e02:1847:: with SMTP id b7mr5900911ilv.180.1632859215623; Tue, 28 Sep 2021 13:00:15 -0700 (PDT) MIME-Version: 1.0 References: <20210927201206.682788-1-lyude@redhat.com> <20210927201206.682788-3-lyude@redhat.com> In-Reply-To: <20210927201206.682788-3-lyude@redhat.com> From: Doug Anderson Date: Tue, 28 Sep 2021 13:00:04 -0700 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [PATCH 2/3] drm/dp, drm/i915: Add support for VESA backlights using PWM for brightness control To: Lyude Paul Cc: Intel Graphics , dri-devel , Rajeev Nandan , Satadru Pramanik , Maarten Lankhorst , Maxime Ripard , Thomas Zimmermann , David Airlie , Daniel Vetter , Jani Nikula , Joonas Lahtinen , Rodrigo Vivi , Ben Skeggs , =?UTF-8?B?VmlsbGUgU3lyasOkbMOk?= , Sean Paul , open list , "open list:DRM DRIVER FOR NVIDIA GEFORCE/QUADRO GPUS" Content-Type: text/plain; charset="UTF-8" Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi, On Mon, Sep 27, 2021 at 1:12 PM Lyude Paul wrote: > > @@ -3305,11 +3313,10 @@ EXPORT_SYMBOL(drm_edp_backlight_enable); > * @bl: Backlight capability info from drm_edp_backlight_init() > * > * This function handles disabling DPCD backlight controls on a panel over AUX. Note that some > - * panels have backlights that are enabled/disabled by other means, despite having their brightness > - * values controlled through DPCD. On such panels &drm_edp_backlight_info.aux_enable will be set to > - * %false, this function will become a no-op (and we will skip updating > - * %DP_EDP_DISPLAY_CONTROL_REGISTER), and the driver must take care to perform it's own > - * implementation specific step for disabling the backlight. > + * panels have backlights that are enabled/disabled via PWM. On such panels > + * &drm_edp_backlight_info.aux_enable will be set to %false, this function will become a no-op (and > + * we will skip updating %DP_EDP_DISPLAY_CONTROL_REGISTER), and the driver must handle disabling the > + * backlight via PWM. I'm not sure I understand the comment above. You say "enabled/disabled via PWM" and that doesn't make sense w/ my mental model. Normally I think of a PWM allowing you to adjust the brightness and there being a separate GPIO that's in charge of enable/disable. To some extent you could think of a PWM as being "disabled" when its duty cycle is 0%, but usually there's separate "enable" logic that really has nothing to do with the PWM itself. In general, it seems like the options are: 1. DPCD controls PWM and the "enable" logic. 2. DPCD controls PWM but requires an external "enable" GPIO. 3. We require an external PWM but DPCD controls the "enable" logic. Maybe you need a second "capability" to describe whether the client of your code knows how to control an enable GPIO? ...or perhaps better you don't need a capability and you can just assume that if the client needs to set an "enable" GPIO that it will do so. That would match how things work today. AKA: a) Client calls the AUX backlight code to "enable" b) AUX backlight code will set the "enable" bit if supported. c) Client will set the "enable" GPIO if it knows about one. Presumably only one of b) or c) will actually do something. If neither does something then this panel simply isn't compatible with this board. > +/** > + * drm_edp_backlight_supported() - Check an eDP DPCD for VESA backlight support > + * @aux: The AUX channel, only used for debug logging > + * @edp_dpcd: The DPCD to check > + * @caps: The backlight capabilities this driver supports > + * > + * Returns: %True if @edp_dpcd indicates that VESA backlight controls are supported, %false > + * otherwise > + */ > +bool drm_edp_backlight_supported(struct drm_dp_aux *aux, > + const u8 edp_dpcd[EDP_DISPLAY_CTL_CAP_SIZE], > + enum drm_edp_backlight_driver_caps caps) > +{ > + if (!(edp_dpcd[1] & DP_EDP_TCON_BACKLIGHT_ADJUSTMENT_CAP)) > + return false; > + > + if (!(caps & DRM_EDP_BACKLIGHT_DRIVER_CAP_PWM) && > + (!(edp_dpcd[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_AUX_SET_CAP) || > + !(edp_dpcd[2] & DP_EDP_BACKLIGHT_AUX_ENABLE_CAP))) { Elsewhere you match DP_EDP_BACKLIGHT_AUX_ENABLE_CAP against edp_dpcd[1]. Here you match against [2]. Are you sure that's correct? > /* > * DisplayPort AUX channel > */ > @@ -2200,7 +2182,11 @@ drm_dp_has_quirk(const struct drm_dp_desc *desc, enum drm_dp_quirk quirk) > * @pwm_freq_pre_divider: The PWM frequency pre-divider value being used for this backlight, if any > * @max: The maximum backlight level that may be set > * @lsb_reg_used: Do we also write values to the DP_EDP_BACKLIGHT_BRIGHTNESS_LSB register? > - * @aux_enable: Does the panel support the AUX enable cap? > + * @aux_enable: Does the panel support the AUX enable cap? Always %false when the driver doesn't > + * support %DRM_EDP_BACKLIGHT_DRIVER_CAP_PWM Why is aux_enable always false if it doesn't support DRM_EDP_BACKLIGHT_DRIVER_CAP_PWM? It doesn't seem like the code enforces this and I'm not sure why it would. Am I confused? From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id AF07AC433F5 for ; Tue, 28 Sep 2021 20:00:20 +0000 (UTC) Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 79EF6610E6 for ; Tue, 28 Sep 2021 20:00:20 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org 79EF6610E6 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=chromium.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=lists.freedesktop.org Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id B658C6E96F; Tue, 28 Sep 2021 20:00:19 +0000 (UTC) Received: from mail-il1-x134.google.com (mail-il1-x134.google.com [IPv6:2607:f8b0:4864:20::134]) by gabe.freedesktop.org (Postfix) with ESMTPS id 655A06E96C for ; Tue, 28 Sep 2021 20:00:18 +0000 (UTC) Received: by mail-il1-x134.google.com with SMTP id b6so337512ilv.0 for ; Tue, 28 Sep 2021 13:00:18 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=tt9tt/F3IxLyQ34cK+9pj6tNPzMMJ1v10W8CXUTZIXc=; b=N3sK4GpDM7NoausvCZ7uvCv2xW8y1o2GqpLxTtHdEz+qukAhK9GRVXI15tRB36rXkG nIIXXm2zgad8mAZpCf5s5YQsjUW2aE4PBDSL5ia/k1Nt9Mo4BFgPQLAOAlIwoYQXGXuH fmr97KMI5Q8zlbFdiIq/dYgMzPBdrUuYP8EE4= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=tt9tt/F3IxLyQ34cK+9pj6tNPzMMJ1v10W8CXUTZIXc=; b=tgCTJqQVCSbaDfDtQyPo8IVCESnyW9ntP0tv3dUBhOHk3RiFep5DXlz4eZfL6olftZ HOHDLrnOZDnQVc6kSvIVd1HmCZnRo1Qu5xKuev5qoEXhqiGZValRSHpbXcBompbXdNNn 17UzziCjYuz8EdFHJRgi3T/H/fE/RAEEkShuZES9joU0WzB7ROKnMBYYA7aX+CxJdvFt TjkRp5dYq3PgW0sSKXRphERiHViYZm1X9RxOUgG/rqXck2RWkdf8+e6xKb6XHA/WX5g9 98/T+Bt/mJ8L2zAX2cR5g+rsg2CI0I3K7V7+L3sXutBPTHUNTOB70pSoQB+GAZqAUjuh SUSA== X-Gm-Message-State: AOAM533w5pqSx8IEFL7cvlpqDk83LldS97M403OFD+m3zePJAKodeGGX 2zPqnFVz6dw++mfkBebMp5hoiFvqnDrTPA== X-Google-Smtp-Source: ABdhPJzF3ZgKPSHiWUgPb+Rw3EvuptQ/jUB9OhP13t6QqmMQWF//UBrhaOeiY5bZX7pEnrCQpOiIYA== X-Received: by 2002:a05:6e02:1c84:: with SMTP id w4mr5846800ill.195.1632859217268; Tue, 28 Sep 2021 13:00:17 -0700 (PDT) Received: from mail-il1-f176.google.com (mail-il1-f176.google.com. [209.85.166.176]) by smtp.gmail.com with ESMTPSA id l25sm27981iob.41.2021.09.28.13.00.16 for (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 28 Sep 2021 13:00:16 -0700 (PDT) Received: by mail-il1-f176.google.com with SMTP id a11so253378ilk.9 for ; Tue, 28 Sep 2021 13:00:16 -0700 (PDT) X-Received: by 2002:a05:6e02:1847:: with SMTP id b7mr5900911ilv.180.1632859215623; Tue, 28 Sep 2021 13:00:15 -0700 (PDT) MIME-Version: 1.0 References: <20210927201206.682788-1-lyude@redhat.com> <20210927201206.682788-3-lyude@redhat.com> In-Reply-To: <20210927201206.682788-3-lyude@redhat.com> From: Doug Anderson Date: Tue, 28 Sep 2021 13:00:04 -0700 X-Gmail-Original-Message-ID: Message-ID: To: Lyude Paul Cc: Intel Graphics , dri-devel , Rajeev Nandan , Satadru Pramanik , Maarten Lankhorst , Maxime Ripard , Thomas Zimmermann , David Airlie , Daniel Vetter , Jani Nikula , Joonas Lahtinen , Rodrigo Vivi , Ben Skeggs , =?UTF-8?B?VmlsbGUgU3lyasOkbMOk?= , Sean Paul , open list , "open list:DRM DRIVER FOR NVIDIA GEFORCE/QUADRO GPUS" Content-Type: text/plain; charset="UTF-8" Subject: Re: [Intel-gfx] [PATCH 2/3] drm/dp, drm/i915: Add support for VESA backlights using PWM for brightness control X-BeenThere: intel-gfx@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Intel graphics driver community testing & development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" Hi, On Mon, Sep 27, 2021 at 1:12 PM Lyude Paul wrote: > > @@ -3305,11 +3313,10 @@ EXPORT_SYMBOL(drm_edp_backlight_enable); > * @bl: Backlight capability info from drm_edp_backlight_init() > * > * This function handles disabling DPCD backlight controls on a panel over AUX. Note that some > - * panels have backlights that are enabled/disabled by other means, despite having their brightness > - * values controlled through DPCD. On such panels &drm_edp_backlight_info.aux_enable will be set to > - * %false, this function will become a no-op (and we will skip updating > - * %DP_EDP_DISPLAY_CONTROL_REGISTER), and the driver must take care to perform it's own > - * implementation specific step for disabling the backlight. > + * panels have backlights that are enabled/disabled via PWM. On such panels > + * &drm_edp_backlight_info.aux_enable will be set to %false, this function will become a no-op (and > + * we will skip updating %DP_EDP_DISPLAY_CONTROL_REGISTER), and the driver must handle disabling the > + * backlight via PWM. I'm not sure I understand the comment above. You say "enabled/disabled via PWM" and that doesn't make sense w/ my mental model. Normally I think of a PWM allowing you to adjust the brightness and there being a separate GPIO that's in charge of enable/disable. To some extent you could think of a PWM as being "disabled" when its duty cycle is 0%, but usually there's separate "enable" logic that really has nothing to do with the PWM itself. In general, it seems like the options are: 1. DPCD controls PWM and the "enable" logic. 2. DPCD controls PWM but requires an external "enable" GPIO. 3. We require an external PWM but DPCD controls the "enable" logic. Maybe you need a second "capability" to describe whether the client of your code knows how to control an enable GPIO? ...or perhaps better you don't need a capability and you can just assume that if the client needs to set an "enable" GPIO that it will do so. That would match how things work today. AKA: a) Client calls the AUX backlight code to "enable" b) AUX backlight code will set the "enable" bit if supported. c) Client will set the "enable" GPIO if it knows about one. Presumably only one of b) or c) will actually do something. If neither does something then this panel simply isn't compatible with this board. > +/** > + * drm_edp_backlight_supported() - Check an eDP DPCD for VESA backlight support > + * @aux: The AUX channel, only used for debug logging > + * @edp_dpcd: The DPCD to check > + * @caps: The backlight capabilities this driver supports > + * > + * Returns: %True if @edp_dpcd indicates that VESA backlight controls are supported, %false > + * otherwise > + */ > +bool drm_edp_backlight_supported(struct drm_dp_aux *aux, > + const u8 edp_dpcd[EDP_DISPLAY_CTL_CAP_SIZE], > + enum drm_edp_backlight_driver_caps caps) > +{ > + if (!(edp_dpcd[1] & DP_EDP_TCON_BACKLIGHT_ADJUSTMENT_CAP)) > + return false; > + > + if (!(caps & DRM_EDP_BACKLIGHT_DRIVER_CAP_PWM) && > + (!(edp_dpcd[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_AUX_SET_CAP) || > + !(edp_dpcd[2] & DP_EDP_BACKLIGHT_AUX_ENABLE_CAP))) { Elsewhere you match DP_EDP_BACKLIGHT_AUX_ENABLE_CAP against edp_dpcd[1]. Here you match against [2]. Are you sure that's correct? > /* > * DisplayPort AUX channel > */ > @@ -2200,7 +2182,11 @@ drm_dp_has_quirk(const struct drm_dp_desc *desc, enum drm_dp_quirk quirk) > * @pwm_freq_pre_divider: The PWM frequency pre-divider value being used for this backlight, if any > * @max: The maximum backlight level that may be set > * @lsb_reg_used: Do we also write values to the DP_EDP_BACKLIGHT_BRIGHTNESS_LSB register? > - * @aux_enable: Does the panel support the AUX enable cap? > + * @aux_enable: Does the panel support the AUX enable cap? Always %false when the driver doesn't > + * support %DRM_EDP_BACKLIGHT_DRIVER_CAP_PWM Why is aux_enable always false if it doesn't support DRM_EDP_BACKLIGHT_DRIVER_CAP_PWM? It doesn't seem like the code enforces this and I'm not sure why it would. Am I confused? From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 559D2C433EF for ; Sat, 2 Oct 2021 02:24:07 +0000 (UTC) Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id E7BED6115C for ; Sat, 2 Oct 2021 02:24:06 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org E7BED6115C Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=chromium.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=lists.freedesktop.org Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id B5DEC6F464; Sat, 2 Oct 2021 02:24:01 +0000 (UTC) Received: from mail-pf1-x42e.google.com (mail-pf1-x42e.google.com [IPv6:2607:f8b0:4864:20::42e]) by gabe.freedesktop.org (Postfix) with ESMTPS id 700736E96C for ; Tue, 28 Sep 2021 20:05:44 +0000 (UTC) Received: by mail-pf1-x42e.google.com with SMTP id 145so19809846pfz.11 for ; Tue, 28 Sep 2021 13:05:44 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=tt9tt/F3IxLyQ34cK+9pj6tNPzMMJ1v10W8CXUTZIXc=; b=N3sK4GpDM7NoausvCZ7uvCv2xW8y1o2GqpLxTtHdEz+qukAhK9GRVXI15tRB36rXkG nIIXXm2zgad8mAZpCf5s5YQsjUW2aE4PBDSL5ia/k1Nt9Mo4BFgPQLAOAlIwoYQXGXuH fmr97KMI5Q8zlbFdiIq/dYgMzPBdrUuYP8EE4= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=tt9tt/F3IxLyQ34cK+9pj6tNPzMMJ1v10W8CXUTZIXc=; b=vEi1orz6se0xbmstTSlJN0JNpgPUc+u3vv9+cZjAHxtFaRtmOOIsCssPYN4JRgZgEF FdcBmKTK9NHKWbmmAbqQvQawGXQ9aiUfRVrTb+eWY0kIZ6pxkLm9TAh2xsjLo8Ukt3LC XEiHaRhuKnJGtuBP06Cnn98CSTB2DiIk5X3PhNMFWKOWyxDnzHlRmvYfoGGVpKWRSbon OUdJ94qBl/Q8GVGw3JW8gJxd7bRH1GTpyYTYOzF5wQ5PydIdBEmum8SS25SNV6zMeFZN /b9syAdlfTZYGjEP1GJwoBF4OV4wzCflqhTqOYjco4Lt2XWT/ja7i1sxFQ3R5TKfOAVG +tUw== X-Gm-Message-State: AOAM532NnQVniTcmeh0X/wGf759FZwffe457vTbMY9afmTeYR55Cufng X1k9PKcT9SmDgMAER7rgoWrQPnT8zhBc+Q== X-Google-Smtp-Source: ABdhPJxySKRj3Ymobn4cW8Zr7jA5mrjXJtwqNzCbmNi+Pwn+9z38p12fB2t6Cw7rOFPXFzcpsvOFFw== X-Received: by 2002:a63:9317:: with SMTP id b23mr6173088pge.199.1632859543620; Tue, 28 Sep 2021 13:05:43 -0700 (PDT) Received: from mail-pg1-f180.google.com (mail-pg1-f180.google.com. [209.85.215.180]) by smtp.gmail.com with ESMTPSA id d3sm8947pfn.156.2021.09.28.13.05.43 for (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 28 Sep 2021 13:05:43 -0700 (PDT) Received: by mail-pg1-f180.google.com with SMTP id m21so183471pgu.13 for ; Tue, 28 Sep 2021 13:05:43 -0700 (PDT) X-Received: by 2002:a05:6e02:1847:: with SMTP id b7mr5900911ilv.180.1632859215623; Tue, 28 Sep 2021 13:00:15 -0700 (PDT) MIME-Version: 1.0 References: <20210927201206.682788-1-lyude@redhat.com> <20210927201206.682788-3-lyude@redhat.com> In-Reply-To: <20210927201206.682788-3-lyude@redhat.com> From: Doug Anderson Date: Tue, 28 Sep 2021 13:00:04 -0700 X-Gmail-Original-Message-ID: Message-ID: To: Lyude Paul Cc: Intel Graphics , dri-devel , Rajeev Nandan , Satadru Pramanik , Maarten Lankhorst , Maxime Ripard , Thomas Zimmermann , David Airlie , Daniel Vetter , Jani Nikula , Joonas Lahtinen , Rodrigo Vivi , Ben Skeggs , =?UTF-8?B?VmlsbGUgU3lyasOkbMOk?= , Sean Paul , open list , "open list:DRM DRIVER FOR NVIDIA GEFORCE/QUADRO GPUS" Content-Type: text/plain; charset="UTF-8" X-Mailman-Approved-At: Sat, 02 Oct 2021 02:24:00 +0000 Subject: Re: [Nouveau] [PATCH 2/3] drm/dp, drm/i915: Add support for VESA backlights using PWM for brightness control X-BeenThere: nouveau@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Nouveau development list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: nouveau-bounces@lists.freedesktop.org Sender: "Nouveau" Hi, On Mon, Sep 27, 2021 at 1:12 PM Lyude Paul wrote: > > @@ -3305,11 +3313,10 @@ EXPORT_SYMBOL(drm_edp_backlight_enable); > * @bl: Backlight capability info from drm_edp_backlight_init() > * > * This function handles disabling DPCD backlight controls on a panel over AUX. Note that some > - * panels have backlights that are enabled/disabled by other means, despite having their brightness > - * values controlled through DPCD. On such panels &drm_edp_backlight_info.aux_enable will be set to > - * %false, this function will become a no-op (and we will skip updating > - * %DP_EDP_DISPLAY_CONTROL_REGISTER), and the driver must take care to perform it's own > - * implementation specific step for disabling the backlight. > + * panels have backlights that are enabled/disabled via PWM. On such panels > + * &drm_edp_backlight_info.aux_enable will be set to %false, this function will become a no-op (and > + * we will skip updating %DP_EDP_DISPLAY_CONTROL_REGISTER), and the driver must handle disabling the > + * backlight via PWM. I'm not sure I understand the comment above. You say "enabled/disabled via PWM" and that doesn't make sense w/ my mental model. Normally I think of a PWM allowing you to adjust the brightness and there being a separate GPIO that's in charge of enable/disable. To some extent you could think of a PWM as being "disabled" when its duty cycle is 0%, but usually there's separate "enable" logic that really has nothing to do with the PWM itself. In general, it seems like the options are: 1. DPCD controls PWM and the "enable" logic. 2. DPCD controls PWM but requires an external "enable" GPIO. 3. We require an external PWM but DPCD controls the "enable" logic. Maybe you need a second "capability" to describe whether the client of your code knows how to control an enable GPIO? ...or perhaps better you don't need a capability and you can just assume that if the client needs to set an "enable" GPIO that it will do so. That would match how things work today. AKA: a) Client calls the AUX backlight code to "enable" b) AUX backlight code will set the "enable" bit if supported. c) Client will set the "enable" GPIO if it knows about one. Presumably only one of b) or c) will actually do something. If neither does something then this panel simply isn't compatible with this board. > +/** > + * drm_edp_backlight_supported() - Check an eDP DPCD for VESA backlight support > + * @aux: The AUX channel, only used for debug logging > + * @edp_dpcd: The DPCD to check > + * @caps: The backlight capabilities this driver supports > + * > + * Returns: %True if @edp_dpcd indicates that VESA backlight controls are supported, %false > + * otherwise > + */ > +bool drm_edp_backlight_supported(struct drm_dp_aux *aux, > + const u8 edp_dpcd[EDP_DISPLAY_CTL_CAP_SIZE], > + enum drm_edp_backlight_driver_caps caps) > +{ > + if (!(edp_dpcd[1] & DP_EDP_TCON_BACKLIGHT_ADJUSTMENT_CAP)) > + return false; > + > + if (!(caps & DRM_EDP_BACKLIGHT_DRIVER_CAP_PWM) && > + (!(edp_dpcd[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_AUX_SET_CAP) || > + !(edp_dpcd[2] & DP_EDP_BACKLIGHT_AUX_ENABLE_CAP))) { Elsewhere you match DP_EDP_BACKLIGHT_AUX_ENABLE_CAP against edp_dpcd[1]. Here you match against [2]. Are you sure that's correct? > /* > * DisplayPort AUX channel > */ > @@ -2200,7 +2182,11 @@ drm_dp_has_quirk(const struct drm_dp_desc *desc, enum drm_dp_quirk quirk) > * @pwm_freq_pre_divider: The PWM frequency pre-divider value being used for this backlight, if any > * @max: The maximum backlight level that may be set > * @lsb_reg_used: Do we also write values to the DP_EDP_BACKLIGHT_BRIGHTNESS_LSB register? > - * @aux_enable: Does the panel support the AUX enable cap? > + * @aux_enable: Does the panel support the AUX enable cap? Always %false when the driver doesn't > + * support %DRM_EDP_BACKLIGHT_DRIVER_CAP_PWM Why is aux_enable always false if it doesn't support DRM_EDP_BACKLIGHT_DRIVER_CAP_PWM? It doesn't seem like the code enforces this and I'm not sure why it would. Am I confused?