Hi,

On Tue, May 25, 2021 at 12:31 AM Rajeev Nandan rajeevny@codeaurora.org wrote:

@@ -171,6 +172,19 @@ struct panel_desc {

    /** @connector_type: LVDS, eDP, DSI, DPI, etc. */
    int connector_type;

•   /**
  

•    * @uses_dpcd_backlight: Panel supports eDP dpcd backlight control.
  

•    *
  

•    * Set true, if the panel supports backlight control over eDP AUX channel
  

•    * using DPCD registers as per VESA's standard.
  

•    */
  

•   bool uses_dpcd_backlight;
  

+};

+struct edp_backlight {

•   struct backlight_device *dev;
  

Can you pick a name other than "dev". In my mind "dev" means you've got a "struct device" or a "struct device *".

•   struct drm_edp_backlight_info info;
  

};

struct panel_simple { @@ -194,6 +208,8 @@ struct panel_simple {

    struct edid *edid;

•   struct edp_backlight *edp_bl;
  

I don't think you need to add this pointer. See below for details, but basically the backlight device should be in base.backlight. Any code that needs the containing structure can use the standard "container_of" syntax.

    struct drm_display_mode override_mode;

    enum drm_panel_orientation orientation;

@@ -330,10 +346,14 @@ static void panel_simple_wait(ktime_t start_ktime, unsigned int min_ms) static int panel_simple_disable(struct drm_panel *panel) { struct panel_simple *p = to_panel_simple(panel);

•   struct edp_backlight *bl = p->edp_bl;
  
    if (!p->enabled)
            return 0;
  
  

•   if (p->desc->uses_dpcd_backlight && bl)
  

•           drm_edp_backlight_disable(p->aux, &bl->info);
  

It feels like this shouldn't be needed. I would have expected that your backlight should be in 'panel->backlight'. Then drm_panel_enable() will call backlight_enable() on your backlight automatically after calling the panel's enable function.

    if (p->desc->delay.disable)
            msleep(p->desc->delay.disable);

@@ -496,6 +516,7 @@ static int panel_simple_prepare(struct drm_panel *panel) static int panel_simple_enable(struct drm_panel *panel) { struct panel_simple *p = to_panel_simple(panel);

•   struct edp_backlight *bl = p->edp_bl;
  
    if (p->enabled)
            return 0;
  

@@ -505,6 +526,10 @@ static int panel_simple_enable(struct drm_panel *panel)

    panel_simple_wait(p->prepared_time, p->desc->delay.prepare_to_enable);

•   if (p->desc->uses_dpcd_backlight && bl)
  

•           drm_edp_backlight_enable(p->aux, &bl->info,
  

•                                    bl->dev->props.brightness);
  

Similar to disable, this shouldn't be needed.

    p->enabled = true;

    return 0;

@@ -565,6 +590,59 @@ static const struct drm_panel_funcs panel_simple_funcs = { .get_timings = panel_simple_get_timings, };

+static int edp_backlight_update_status(struct backlight_device *bd) +{

•   struct panel_simple *p = bl_get_data(bd);
  

•   struct edp_backlight *bl = p->edp_bl;
  

•   if (!p->enabled)
  

•           return 0;
  

•   return drm_edp_backlight_set_level(p->aux, &bl->info, bd->props.brightness);
  

I notice that the "nouveau" driver grabs a whole pile of locks around this. Do we need some of those? I guess perhaps checking "p->enabled" isn't so valid without holding some of those locks.

Actually, I guess you probably can't look at "p->enabled" anyway if this gets moved out of panel-simple as I'm suggesting.

...but do you even need something like this check? Shouldn't it be handled by the fact that drm_panel will handle enabling/disabling the backlight at the right times?

+}

+static const struct backlight_ops edp_backlight_ops = {

•   .update_status = edp_backlight_update_status,
  

+};

+static int edp_backlight_register(struct device *dev, struct panel_simple *panel) +{

•   struct edp_backlight *bl;
  

•   struct backlight_properties props = { 0 };
  

•   u16 current_level;
  

•   u8 current_mode;
  

•   u8 edp_dpcd[EDP_DISPLAY_CTL_CAP_SIZE];
  

•   int ret;
  

•   bl = devm_kzalloc(dev, sizeof(*bl), GFP_KERNEL);
  

•   if (!bl)
  

•           return -ENOMEM;
  

•   ret = drm_dp_dpcd_read(panel->aux, DP_EDP_DPCD_REV, edp_dpcd,
  

•                          EDP_DISPLAY_CTL_CAP_SIZE);
  

•   if (ret < 0)
  

•           return ret;
  

•   ret = drm_edp_backlight_init(panel->aux, &bl->info, 0, edp_dpcd,
  

•                                &current_level, &current_mode);
  

•   if (ret < 0)
  

•           return ret;
  

•   props.type = BACKLIGHT_RAW;
  

•   props.brightness = current_level;
  

•   props.max_brightness = bl->info.max;
  

•   bl->dev = devm_backlight_device_register(dev, "edp_backlight",
  

•                                           dev, panel,
  

•                                           &edp_backlight_ops, &props);
  

•   if (IS_ERR(bl->dev))
  

•           return PTR_ERR(bl->dev);
  

•   panel->edp_bl = bl;
  

•   return 0;
  

+}

I expect there to be quite a bit of pushback to putting this directly into panel-simple. How about if you move edp_backlight_register() into drm_panel.c, parallel to drm_panel_of_backlight(). Maybe you'd call it drm_panel_dp_aux_backlight() to make it look symmetric?

If you do that then the amount of code / complexity being added to "simple" panel is quite small. I think it would just come down to adding the boolean flag and the patch to probe that you have below.

Actually, now that I think about it, you could maybe even get by _without_ the boolean flag? I think you could use these rules (untested!):

1. Call drm_panel_of_backlight() always, just like we do today. If a backlight was specified in the device tree then we should use it.

2. If no backlight was specified in the device tree then, I believe, drm_panel_of_backlight() will return with no errors but will have panel->backlight set to NULL.

3. If there was no backlight specified in the device tree and you have the DP AUX channel and drm_edp_backlight_supported() then create a DP AUX backlight.

The one feature that wouldn't be supported by the above would be "DP_EDP_BACKLIGHT_AUX_PWM_PRODUCT_CAP". Presumably that's fine. If someone later wants to figure out how to solve that then they can.

static struct panel_desc panel_dpi;

static int panel_dpi_probe(struct device *dev, @@ -796,9 +874,24 @@ static int panel_simple_probe(struct device *dev, const struct panel_desc *desc,

    drm_panel_init(&panel->base, dev, &panel_simple_funcs, connector_type);

•   err = drm_panel_of_backlight(&panel->base);
  

•   if (err)
  

•           goto disable_pm_runtime;
  

•   if (panel->desc->uses_dpcd_backlight) {
  

•           if (!panel->aux) {
  

•                   dev_err(dev, "edp backlight needs DP aux\n");
  

•                   err = -EINVAL;
  

•                   goto disable_pm_runtime;
  

•           }
  

•           err = edp_backlight_register(dev, panel);
  

•           if (err) {
  

•                   dev_err(dev, "failed to register edp backlight %d\n", err);
  

•                   goto disable_pm_runtime;
  

•           }
  

•   } else {
  

nit: get rid of the blank line above the "} else {"

•           err = drm_panel_of_backlight(&panel->base);
  

•           if (err)
  

•                   goto disable_pm_runtime;
  

•   }
  

See above where I'm suggesting some different logic. Specifically: always try the drm_panel_of_backlight() call and then fallback to the AUX backlight if "panel->base.backlight" is NULL and "panel->aux" is not NULL.

-Doug

Hi,

On Thu, May 27, 2021 at 5:21 AM rajeevny@codeaurora.org wrote:

...

...

@@ -171,6 +172,19 @@ struct panel_desc {

    /** @connector_type: LVDS, eDP, DSI, DPI, etc. */
    int connector_type;

•   /**
  

•    * @uses_dpcd_backlight: Panel supports eDP dpcd backlight
  

control.

•    *
  

•    * Set true, if the panel supports backlight control over eDP
  

AUX channel

•    * using DPCD registers as per VESA's standard.
  

•    */
  

•   bool uses_dpcd_backlight;
  

+};

+struct edp_backlight {

•   struct backlight_device *dev;
  

Can you pick a name other than "dev". In my mind "dev" means you've got a "struct device" or a "struct device *".

In the backlight.h "bd" is used for "struct backlight_device". I can use "bd"?

That would be OK w/ me since it's not "dev". In theory you could also call it "base" like panel-simple does with the base class drm_panel, but I'll leave that up to you. It's mostly that in my brain "dev" is reserved for "struct device" but otherwise I'm pretty flexible.

...

...

•   struct drm_edp_backlight_info info;
  

};

struct panel_simple { @@ -194,6 +208,8 @@ struct panel_simple {

    struct edid *edid;

•   struct edp_backlight *edp_bl;
  

I don't think you need to add this pointer. See below for details, but basically the backlight device should be in base.backlight. Any code that needs the containing structure can use the standard "container_of" syntax.

The documentation of the "struct drm_panel -> backlight" mentions "backlight is set by drm_panel_of_backlight() and drivers shall not assign it." That's why I was not sure if I should touch that part. Because of this, I added backlight enable/disable calls inside panel_simple_disable/enable().

Fair enough. In my opinion (subject to being overridden by the adults in the room), if you move your backlight code into drm_panel.c and call it drm_panel_dp_aux_backlight() then it's fair game to use. This basically means that it's no longer a "driver" assigning it since it's being done in drm_panel.c. ;-) Obviously you'd want to update the comment, too...

...

...

•           err = drm_panel_of_backlight(&panel->base);
  

•           if (err)
  

•                   goto disable_pm_runtime;
  

•   }
  

See above where I'm suggesting some different logic. Specifically: always try the drm_panel_of_backlight() call and then fallback to the AUX backlight if "panel->base.backlight" is NULL and "panel->aux" is not NULL.

What I understood:

1. Create a new API drm_panel_dp_aux_backlight() in drm_panel.c

1.1. Register DP AUX backlight if "struct drm_dp_aux" is given and drm_edp_backlight_supported() 2. Create a call back function for backlight ".update_status()" inside drm_panel.c ? This function should also handle the backlight enable/disable operations. 3. Use the suggested rules to call drm_panel_dp_aux_backlight() as a fallback, if no backlight is specified in the DT. 4. Remove the @uses_dpcd_backlight flag from panel_desc as this should be auto-detected.

This sounds about right to me.

As per all of my reviews in the DRM subsystem, this is all just my opinion and if someone more senior in DRM contradicts me then, of course, you might have to change directions. Hopefully that doesn't happen but it's always good to give warning...

-Doug

oh-looks like my patches just got reviewed, so hopefully I should get a chance to get a look at this in the next day or two :)

On Tue, 2021-05-25 at 13:00 +0530, Rajeev Nandan wrote:

Add basic support of panel backlight control over eDP aux channel using VESA's standard backlight control interface.

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)

Changes in v4:

• New

[1] https://lore.kernel.org/dri-devel/20210524165920.v8.7.I18e60221f6d048d14d6c5...

drivers/gpu/drm/panel/panel-simple.c | 99 ++++++++++++++++++++++++++++++++++--  1 file changed, 96 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c index b09be6e..f9e4e60 100644 --- a/drivers/gpu/drm/panel/panel-simple.c +++ b/drivers/gpu/drm/panel/panel-simple.c @@ -21,6 +21,7 @@   * DEALINGS IN THE SOFTWARE.   */   +#include <linux/backlight.h>  #include <linux/delay.h>  #include <linux/gpio/consumer.h>  #include <linux/iopoll.h> @@ -171,6 +172,19 @@ struct panel_desc {           /** @connector_type: LVDS, eDP, DSI, DPI, etc. */         int connector_type;

+       /** +        * @uses_dpcd_backlight: Panel supports eDP dpcd backlight control. +        * +        * Set true, if the panel supports backlight control over eDP AUX channel +        * using DPCD registers as per VESA's standard. +        */ +       bool uses_dpcd_backlight; +};

+struct edp_backlight { +       struct backlight_device *dev; +       struct drm_edp_backlight_info info;  };    struct panel_simple { @@ -194,6 +208,8 @@ struct panel_simple {           struct edid *edid;   +       struct edp_backlight *edp_bl;

struct drm_display_mode override_mode;           enum drm_panel_orientation orientation; @@ -330,10 +346,14 @@ static void panel_simple_wait(ktime_t start_ktime, unsigned int min_ms)  static int panel_simple_disable(struct drm_panel *panel)  {         struct panel_simple *p = to_panel_simple(panel); +       struct edp_backlight *bl = p->edp_bl;           if (!p->enabled)                 return 0;   +       if (p->desc->uses_dpcd_backlight && bl) +               drm_edp_backlight_disable(p->aux, &bl->info);

if (p->desc->delay.disable)                 msleep(p->desc->delay.disable);   @@ -496,6 +516,7 @@ static int panel_simple_prepare(struct drm_panel *panel)  static int panel_simple_enable(struct drm_panel *panel)  {         struct panel_simple *p = to_panel_simple(panel); +       struct edp_backlight *bl = p->edp_bl;           if (p->enabled)                 return 0; @@ -505,6 +526,10 @@ static int panel_simple_enable(struct drm_panel *panel)           panel_simple_wait(p->prepared_time, p->desc-

...

delay.prepare_to_enable);

+       if (p->desc->uses_dpcd_backlight && bl) +               drm_edp_backlight_enable(p->aux, &bl->info, +                                        bl->dev->props.brightness);

p->enabled = true;           return 0; @@ -565,6 +590,59 @@ static const struct drm_panel_funcs panel_simple_funcs = {         .get_timings = panel_simple_get_timings,  };   +static int edp_backlight_update_status(struct backlight_device *bd) +{ +       struct panel_simple *p = bl_get_data(bd); +       struct edp_backlight *bl = p->edp_bl;

+       if (!p->enabled) +               return 0;

+       return drm_edp_backlight_set_level(p->aux, &bl->info, bd-

...

props.brightness);

+}

+static const struct backlight_ops edp_backlight_ops = { +       .update_status = edp_backlight_update_status, +};

+static int edp_backlight_register(struct device *dev, struct panel_simple *panel) +{ +       struct edp_backlight *bl; +       struct backlight_properties props = { 0 }; +       u16 current_level; +       u8 current_mode; +       u8 edp_dpcd[EDP_DISPLAY_CTL_CAP_SIZE]; +       int ret;

+       bl = devm_kzalloc(dev, sizeof(*bl), GFP_KERNEL); +       if (!bl) +               return -ENOMEM;

+       ret = drm_dp_dpcd_read(panel->aux, DP_EDP_DPCD_REV, edp_dpcd, +                              EDP_DISPLAY_CTL_CAP_SIZE); +       if (ret < 0) +               return ret;

+       ret = drm_edp_backlight_init(panel->aux, &bl->info, 0, edp_dpcd, +                                    &current_level, &current_mode); +       if (ret < 0) +               return ret;

+       props.type = BACKLIGHT_RAW; +       props.brightness = current_level; +       props.max_brightness = bl->info.max;

+       bl->dev = devm_backlight_device_register(dev, "edp_backlight", +                                               dev, panel, +                                               &edp_backlight_ops, &props); +       if (IS_ERR(bl->dev)) +               return PTR_ERR(bl->dev);

+       panel->edp_bl = bl;

+       return 0; +}

static struct panel_desc panel_dpi;    static int panel_dpi_probe(struct device *dev, @@ -796,9 +874,24 @@ static int panel_simple_probe(struct device *dev, const struct panel_desc *desc,           drm_panel_init(&panel->base, dev, &panel_simple_funcs, connector_type);   -       err = drm_panel_of_backlight(&panel->base); -       if (err) -               goto disable_pm_runtime; +       if (panel->desc->uses_dpcd_backlight) { +               if (!panel->aux) { +                       dev_err(dev, "edp backlight needs DP aux\n"); +                       err = -EINVAL; +                       goto disable_pm_runtime; +               }

+               err = edp_backlight_register(dev, panel); +               if (err) { +                       dev_err(dev, "failed to register edp backlight %d\n", err); +                       goto disable_pm_runtime; +               }

+       } else { +               err = drm_panel_of_backlight(&panel->base); +               if (err) +                       goto disable_pm_runtime; +       }           drm_panel_add(&panel->base);

Hi,

On Tue, May 25, 2021 at 12:31 AM Rajeev Nandan rajeevny@codeaurora.org wrote:

Some panels datasheets may specify a delay between the enable GPIO and the regulator. Support this in panel-simple.

Signed-off-by: Rajeev Nandan rajeevny@codeaurora.org

Changes in v4:

• New

drivers/gpu/drm/panel/panel-simple.c | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+)

diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c index f9e4e60..caed71b 100644 --- a/drivers/gpu/drm/panel/panel-simple.c +++ b/drivers/gpu/drm/panel/panel-simple.c @@ -134,6 +134,22 @@ struct panel_desc { unsigned int prepare_to_enable;

            /**

•            * @delay.power_to_enable: Time for the power to enable the display on.
  

•            *
  

•            * The time (in milliseconds) that it takes for the panel to
  

•            * turn the display on.
  

Maybe a slightly better description:

The time (in milliseconds) to wait after powering up the display before asserting its enable pin.

•            */
  

•           unsigned int power_to_enable;
  

•           /**
  

•            * @delay.disable_to_power_off: Time for the disable to power the display off.
  

•            *
  

•            * The time (in milliseconds) that it takes for the panel to
  

•            * turn the display off.
  

Maybe a slightly better description:

The time (in milliseconds) to wait after disabling the display before deasserting its enable pin.

•            */
  

•           unsigned int disable_to_power_off;
  

•           /**
             * @delay.enable: Time for the panel to display a valid frame.
             *
             * The time (in milliseconds) that it takes for the panel to
  

@@ -367,6 +383,10 @@ static int panel_simple_suspend(struct device *dev) struct panel_simple *p = dev_get_drvdata(dev);

    gpiod_set_value_cansleep(p->enable_gpio, 0);

•   if (p->desc->delay.disable_to_power_off)
  

•           msleep(p->desc->delay.disable_to_power_off);
  

I wonder if it's worth a warning if "p->desc->delay.disable_to_power_off" is non-zero and p->enable_gpio is NULL? I guess in theory it'd also be nice to confirm that p->supply wasn't a dummy regulator, but that's slightly harder.

    regulator_disable(p->supply);
    p->unprepared_time = ktime_get();

@@ -427,6 +447,9 @@ static int panel_simple_prepare_once(struct panel_simple *p) return err; }

•   if (p->desc->delay.power_to_enable)
  

•           msleep(p->desc->delay.power_to_enable);
  

Similar to above: I wonder if it's worth a warning if "p->desc->delay.power_to_enable" is non-zero and p->enable_gpio is NULL?

-Doug

Hi,

On Tue, May 25, 2021 at 12:31 AM Rajeev Nandan rajeevny@codeaurora.org wrote:

Add Samsung 13.3" FHD eDP AMOLED panel.

Signed-off-by: Rajeev Nandan rajeevny@codeaurora.org

Changes in v4:

• New

Documentation/devicetree/bindings/display/panel/panel-simple.yaml | 2 ++ 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/display/panel/panel-simple.yaml b/Documentation/devicetree/bindings/display/panel/panel-simple.yaml index 4a0a5e1..f5acfd6 100644 --- a/Documentation/devicetree/bindings/display/panel/panel-simple.yaml +++ b/Documentation/devicetree/bindings/display/panel/panel-simple.yaml @@ -242,6 +242,8 @@ properties: - rocktech,rk101ii01d-ct # Rocktech Display Ltd. RK070ER9427 800(RGB)x480 TFT LCD panel - rocktech,rk070er9427

•    # Samsung 13.3" FHD (1920x1080 pixels) eDP AMOLED panel
  

•  - samsung,atna33xc20
     # Samsung 12.2" (2560x1600 pixels) TFT LCD panel
   - samsung,lsn122dl01-c01
  

This panel is slightly different from other panels currently listed here because it requires the DP AUX channel to control the backlight. However, in my mind, it still qualifies as "simple" because this fact is probable and no extra dt properties are needed. Thus:

Reviewed-by: Douglas Anderson dianders@chromium.org

Hi,

On Tue, May 25, 2021 at 12:31 AM Rajeev Nandan rajeevny@codeaurora.org wrote:

Add Samsung 13.3" FHD eDP AMOLED panel.

Signed-off-by: Rajeev Nandan rajeevny@codeaurora.org

Changes in v4:

• New

drivers/gpu/drm/panel/panel-simple.c | 34 ++++++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+)

diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c index caed71b..21af794 100644 --- a/drivers/gpu/drm/panel/panel-simple.c +++ b/drivers/gpu/drm/panel/panel-simple.c @@ -3644,6 +3644,37 @@ static const struct panel_desc rocktech_rk101ii01d_ct = { .connector_type = DRM_MODE_CONNECTOR_LVDS, };

+static const struct drm_display_mode samsung_atna33xc20_mode = {

•   .clock = 138770,
  

•   .hdisplay = 1920,
  

•   .hsync_start = 1920 + 48,
  

•   .hsync_end = 1920 + 48 + 32,
  

•   .htotal = 1920 + 48 + 32 + 80,
  

•   .vdisplay = 1080,
  

•   .vsync_start = 1080 + 8,
  

•   .vsync_end = 1080 + 8 + 8,
  

•   .vtotal = 1080 + 8 + 8 + 16,
  

•   .flags = DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_NVSYNC,
  

+};

+static const struct panel_desc samsung_atna33xc20 = {

•   .modes = &samsung_atna33xc20_mode,
  

•   .num_modes = 1,
  

•   .bpc = 10,
  

•   .size = {
  

•           .width = 294,
  

•           .height = 165,
  

•   },
  

•   .delay = {
  

•           .disable_to_power_off = 150,
  

•           .power_to_enable = 150,
  

•           .hpd_absent_delay = 200,
  

•           .unprepare = 500,
  

•   },
  

•   .connector_type = DRM_MODE_CONNECTOR_eDP,
  

•   .uses_dpcd_backlight = true,
  

From my feedback on the previous patch in this series, I believe the

"uses_dpcd_backlight" property should be removed and this should be auto-detected. Other than that this patch looks fine to me. Feel free to add my Reviewed-by tag next spin when that property is removed.