Hi,

On Sun, May 30, 2021 at 8:57 AM Rajeev Nandan [off-list ref] 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.

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