linux-fbdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* Re: [v5 1/5] drm/panel: add basic DP AUX backlight support
       [not found] ` <1622390172-31368-2-git-send-email-rajeevny@codeaurora.org>
@ 2021-06-03  0:05   ` Doug Anderson
  2021-06-03  6:26     ` rajeevny
  0 siblings, 1 reply; 3+ messages in thread
From: Doug Anderson @ 2021-06-03  0:05 UTC (permalink / raw)
  To: Rajeev Nandan
  Cc: dri-devel, linux-arm-msm, freedreno,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, LKML,
	Thierry Reding, Sam Ravnborg, Rob Clark, Lyude Paul, Jani Nikula,
	Rob Herring, Laurent Pinchart, Andrzej Hajda, Daniel Thompson,
	Kristian H. Kristensen, Abhinav Kumar, Sean Paul, Kalyan Thota,
	Krishna Manikandan, Lee Jones, Jingoo Han, linux-fbdev

Hi,

On Sun, May 30, 2021 at 8:57 AM Rajeev Nandan <rajeevny@codeaurora.org> wrote:
>
> +static int dp_aux_backlight_update_status(struct backlight_device *bd)
> +{
> +       struct dp_aux_backlight *bl = bl_get_data(bd);
> +       u16 brightness = backlight_get_brightness(bd);
> +       int ret = 0;
> +
> +       if (brightness > 0) {
> +               if (!bl->enabled) {
> +                       drm_edp_backlight_enable(bl->aux, &bl->info, brightness);
> +                       bl->enabled = true;
> +                       return 0;
> +               }
> +               ret = drm_edp_backlight_set_level(bl->aux, &bl->info, brightness);
> +       } else {
> +               if (bl->enabled) {
> +                       drm_edp_backlight_disable(bl->aux, &bl->info);
> +                       bl->enabled = false;
> +               }
> +       }

I was trying to figure out if there are any races / locking problems /
problems trying to tweak the backlight when the panel is off. I don't
_think_ there are. Specifically:

1. Before turning the panel off, drm_panel will call
backlight_disable(). That will set BL_CORE_FBBLANK which is not set by
any other calls. Then it will call your
dp_aux_backlight_update_status().

2. Once BL_CORE_FBBLANK is set then backlight_get_brightness() will
always return 0.

This means that a transition from 0 -> non-zero (and enable) will
always only happen when the panel is on, which is good. It also means
that we'll always transition to 0 (disable the backlight) when the
panel turns off.

In terms of other races, it looks like the AUX transfer code handles
grabbing a mutex around transfers so we should be safe there.

So I guess the above is just a long-winded way of saying that this
looks right to me. :-)

BTW: we should probably make sure that the full set of people
identified by `./scripts/get_maintainer.pl -f
./drivers/video/backlight` are CCed on your series. I see Daniel
already and I've added the rest.


> +/**
> + * drm_panel_dp_aux_backlight - create and use DP AUX backlight
> + * @panel: DRM panel
> + * @aux: The DP AUX channel to use
> + *
> + * Use this function to create and handle backlight if your panel
> + * supports backlight control over DP AUX channel using DPCD
> + * registers as per VESA's standard backlight control interface.
> + *
> + * When the panel is enabled backlight will be enabled after a
> + * successful call to &drm_panel_funcs.enable()
> + *
> + * When the panel is disabled backlight will be disabled before the
> + * call to &drm_panel_funcs.disable().
> + *
> + * A typical implementation for a panel driver supporting backlight
> + * control over DP AUX will call this function at probe time.
> + * Backlight will then be handled transparently without requiring
> + * any intervention from the driver.
> + *
> + * drm_panel_dp_aux_backlight() must be called after the call to drm_panel_init().
> + *
> + * Return: 0 on success or a negative error code on failure.
> + */
> +int drm_panel_dp_aux_backlight(struct drm_panel *panel, struct drm_dp_aux *aux)
> +{
> +       struct dp_aux_backlight *bl;
> +       struct backlight_properties props = { 0 };
> +       u16 current_level;
> +       u8 current_mode;
> +       u8 edp_dpcd[EDP_DISPLAY_CTL_CAP_SIZE];
> +       int ret;
> +
> +       if (!panel || !panel->dev || !aux)
> +               return -EINVAL;
> +
> +       bl = devm_kzalloc(panel->dev, sizeof(*bl), GFP_KERNEL);
> +       if (!bl)
> +               return -ENOMEM;
> +
> +       bl->aux = aux;
> +
> +       ret = drm_dp_dpcd_read(aux, DP_EDP_DPCD_REV, edp_dpcd,
> +                              EDP_DISPLAY_CTL_CAP_SIZE);
> +       if (ret < 0)
> +               return ret;
> +
> +       if (!drm_edp_backlight_supported(edp_dpcd)) {
> +               DRM_DEV_INFO(panel->dev, "DP AUX backlight is not supported\n");
> +               return 0;
> +       }

nit: move this part up above the memory allocation. There's no reason
to allocate memory for "bl" if the backlight isn't supported.


> @@ -64,8 +65,8 @@ enum drm_panel_orientation;
>   * the panel. This is the job of the .unprepare() function.
>   *
>   * Backlight can be handled automatically if configured using
> - * drm_panel_of_backlight(). Then the driver does not need to implement the
> - * functionality to enable/disable backlight.
> + * drm_panel_of_backlight() or drm_panel_dp_aux_backlight(). Then the driver
> + * does not need to implement the functionality to enable/disable backlight.
>   */
>  struct drm_panel_funcs {
>         /**
> @@ -144,8 +145,8 @@ struct drm_panel {
>          * Backlight device, used to turn on backlight after the call
>          * to enable(), and to turn off backlight before the call to
>          * disable().
> -        * backlight is set by drm_panel_of_backlight() and drivers
> -        * shall not assign it.
> +        * backlight is set by drm_panel_of_backlight()/drm_panel_dp_aux_backlight()
> +        * and drivers shall not assign it.

Slight nit that I would have wrapped the drm_panel_dp_aux_backlight()
to the next line just to avoid one really long line followed by a
short one.

Other than the two nits (ordering of memory allocation and word
wrapping in a comment), this looks good to me. Feel free to add my
Reviewed-by tag when you fix the nits.

NOTE: Even though I have commit access to drm-misc now, I wouldn't
feel comfortable merging this to drm-misc myself without review
feedback from someone more senior. Obviously we're still blocked on my
and Lyude's series landing first, but even assuming those just land
as-is we'll need some more adult supervision before this can land. ;-)
That being said, I personally think this looks pretty nice now.


-Doug



>          */
>         struct backlight_device *backlight;
>
> @@ -208,11 +209,17 @@ static inline int of_drm_get_panel_orientation(const struct device_node *np,
>  #if IS_ENABLED(CONFIG_DRM_PANEL) && (IS_BUILTIN(CONFIG_BACKLIGHT_CLASS_DEVICE) || \
>         (IS_MODULE(CONFIG_DRM) && IS_MODULE(CONFIG_BACKLIGHT_CLASS_DEVICE)))
>  int drm_panel_of_backlight(struct drm_panel *panel);
> +int drm_panel_dp_aux_backlight(struct drm_panel *panel, struct drm_dp_aux *aux);
>  #else
>  static inline int drm_panel_of_backlight(struct drm_panel *panel)
>  {
>         return 0;
>  }
> +static inline int drm_panel_dp_aux_backlight(struct drm_panel *panel,
> +                                            struct drm_dp_aux *aux)
> +{
> +       return 0;
> +}
>  #endif
>
>  #endif
> --
> 2.7.4
>

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [v5 2/5] drm/panel-simple: Support DP AUX backlight
       [not found] ` <1622390172-31368-3-git-send-email-rajeevny@codeaurora.org>
@ 2021-06-03  0:05   ` Doug Anderson
  0 siblings, 0 replies; 3+ messages in thread
From: Doug Anderson @ 2021-06-03  0:05 UTC (permalink / raw)
  To: Rajeev Nandan
  Cc: dri-devel, linux-arm-msm, freedreno,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, LKML,
	Thierry Reding, Sam Ravnborg, Rob Clark, Lyude Paul, Jani Nikula,
	Rob Herring, Laurent Pinchart, Andrzej Hajda, Daniel Thompson,
	Kristian H. Kristensen, Abhinav Kumar, Sean Paul, Kalyan Thota,
	Krishna Manikandan, Lee Jones, Jingoo Han, linux-fbdev

Hi,

On Sun, May 30, 2021 at 8:57 AM Rajeev Nandan <rajeevny@codeaurora.org> wrote:
>
> If there is no backlight specified in the device tree and the panel
> has access to the DP AUX channel then create a DP AUX backlight if
> supported by the panel.
>
> Signed-off-by: Rajeev Nandan <rajeevny@codeaurora.org>
> ---
>
> This patch depends on [1] (drm/panel: panel-simple: Stash DP AUX bus;
> allow using it for DDC) and the previous patch (2/5) of this series.
>
> Changes in v4:
> - New
>
> Changes in v5:
> - Address review comments and move backlight functions to drm_panel.c (Douglas)
> - Create and register DP AUX backlight if there is no backlight specified in the
>   device tree and panel has the DP AUX channel. (Douglas)
> - The new drm_panel_dp_aux_backlight() will do the drm_edp_backlight_supported() check.
>
> [1] https://lore.kernel.org/dri-devel/20210524165920.v8.7.I18e60221f6d048d14d6c50a770b15f356fa75092@changeid/
>
>  drivers/gpu/drm/panel/panel-simple.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
> index b09be6e..047fad5 100644
> --- a/drivers/gpu/drm/panel/panel-simple.c
> +++ b/drivers/gpu/drm/panel/panel-simple.c
> @@ -800,6 +800,12 @@ static int panel_simple_probe(struct device *dev, const struct panel_desc *desc,
>         if (err)
>                 goto disable_pm_runtime;
>
> +       if (!panel->base.backlight && panel->aux) {
> +               err = drm_panel_dp_aux_backlight(&panel->base, panel->aux);
> +               if (err)
> +                       goto disable_pm_runtime;
> +       }

It's so nice now!

Reviewed-by: Douglas Anderson <dianders@chromium.org>

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [v5 1/5] drm/panel: add basic DP AUX backlight support
  2021-06-03  0:05   ` [v5 1/5] drm/panel: add basic DP AUX backlight support Doug Anderson
@ 2021-06-03  6:26     ` rajeevny
  0 siblings, 0 replies; 3+ messages in thread
From: rajeevny @ 2021-06-03  6:26 UTC (permalink / raw)
  To: Doug Anderson
  Cc: dri-devel, linux-arm-msm, freedreno,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, LKML,
	Thierry Reding, Sam Ravnborg, Rob Clark, Lyude Paul, Jani Nikula,
	Rob Herring, Laurent Pinchart, Andrzej Hajda, Daniel Thompson,
	Kristian H. Kristensen, Abhinav Kumar, Sean Paul, Kalyan Thota,
	Krishna Manikandan, Lee Jones, Jingoo Han, linux-fbdev

On 03-06-2021 05:35, Doug Anderson wrote:
> Hi,
> 
> On Sun, May 30, 2021 at 8:57 AM Rajeev Nandan <rajeevny@codeaurora.org> 
> wrote:
>> 

> 
> Other than the two nits (ordering of memory allocation and word
> wrapping in a comment), this looks good to me. Feel free to add my
> Reviewed-by tag when you fix the nits.
> 
> NOTE: Even though I have commit access to drm-misc now, I wouldn't
> feel comfortable merging this to drm-misc myself without review
> feedback from someone more senior. Obviously we're still blocked on my
> and Lyude's series landing first, but even assuming those just land
> as-is we'll need some more adult supervision before this can land. ;-)
> That being said, I personally think this looks pretty nice now.
> 
> 
> -Doug

Thank you, Doug.

I'll address the review comments of this patch and another patch (v5 
3/5)
in the next spin. I'll wait for Lyude to check this series, as she 
wanted
to review it in a few days.


Thanks,
Rajeev


^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2021-06-03  6:26 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <1622390172-31368-1-git-send-email-rajeevny@codeaurora.org>
     [not found] ` <1622390172-31368-2-git-send-email-rajeevny@codeaurora.org>
2021-06-03  0:05   ` [v5 1/5] drm/panel: add basic DP AUX backlight support Doug Anderson
2021-06-03  6:26     ` rajeevny
     [not found] ` <1622390172-31368-3-git-send-email-rajeevny@codeaurora.org>
2021-06-03  0:05   ` [v5 2/5] drm/panel-simple: Support DP AUX backlight Doug Anderson

This is a public inbox, see mirroring instructions
on how to clone and mirror all data and code used for this inbox