[v4 0/4] drm: Support basic DPCD backlight in panel-simple and add a new panel ATNA33XC20

[v4 0/4] drm: Support basic DPCD backlight in panel-simple and add a new panel ATNA33XC20

[Rajeev Nandan]

This series adds the support for the eDP panel that needs the backlight
controlling over the DP AUX channel using DPCD registers of the panel
as per the VESA's standard.

This series also adds support for the Samsung eDP AMOLED panel that
needs DP AUX to control the backlight, and introduces new delays in the
@panel_desc.delay to support this panel.

This patch series depends on the following two series:
- Doug's series [1], exposed the DP AUX channel to the panel-simple.
- Lyude's series [2], introduced new drm helper functions for DPCD
  backlight.

This series is the logical successor to the series [3].

Changes in v1:
- Created dpcd backlight helper with very basic functionality, added
  backlight registration in the ti-sn65dsi86 bridge driver.

Changes in v2:
- Created a new DisplayPort aux backlight driver and moved the code from
  drm_dp_aux_backlight.c (v1) to the new driver.

Changes in v3:
- Fixed module compilation (kernel test bot).

Changes in v4:
- Added basic DPCD backlight support in panel-simple.
- Added support for a new Samsung panel ATNA33XC20 that needs DPCD
  backlight controlling and has a requirement of delays between enable
  GPIO and regulator.

[1] https://lore.kernel.org/dri-devel/20210525000159.3384921-1-dianders@chromium.org/
[2] https://lore.kernel.org/dri-devel/20210514181504.565252-1-lyude@redhat.com/
[3] https://lore.kernel.org/dri-devel/1619416756-3533-1-git-send-email-rajeevny@codeaurora.org/

Rajeev Nandan (4):
  drm/panel-simple: Add basic DPCD backlight support
  drm/panel-simple: Support for delays between GPIO & regulator
  dt-bindings: display: simple: Add Samsung ATNA33XC20
  drm/panel-simple: Add Samsung ATNA33XC20

 .../bindings/display/panel/panel-simple.yaml       |   2 +
 drivers/gpu/drm/panel/panel-simple.c               | 156 ++++++++++++++++++++-
 2 files changed, 155 insertions(+), 3 deletions(-)

-- 
2.7.4

[v4 1/4] drm/panel-simple: Add basic DPCD backlight support

[Rajeev Nandan]

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.I18e60221f6d048d14d6c50a770b15f356fa75092@changeid/

 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);
 
-- 
2.7.4

Re: [v4 1/4] drm/panel-simple: Add basic DPCD backlight support

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 11483 bytes --]

Hi Rajeev,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on drm-tip/drm-tip]
[also build test ERROR on next-20210525]
[cannot apply to robh/for-next drm-intel/for-linux-next drm-exynos/exynos-drm-next tegra-drm/drm/tegra/for-next linus/master drm/drm-next v5.13-rc3]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Rajeev-Nandan/drm-Support-basic-DPCD-backlight-in-panel-simple-and-add-a-new-panel-ATNA33XC20/20210525-153326
base:   git://anongit.freedesktop.org/drm/drm-tip drm-tip
config: arm-randconfig-r025-20210525 (attached as .config)
compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project 99155e913e9bad5f7f8a247f8bb3a3ff3da74af1)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install arm cross compiling tool for clang build
        # apt-get install binutils-arm-linux-gnueabi
        # https://github.com/0day-ci/linux/commit/24e7ccb98951b0b4c7ae8a367273f8e73c074804
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Rajeev-Nandan/drm-Support-basic-DPCD-backlight-in-panel-simple-and-add-a-new-panel-ATNA33XC20/20210525-153326
        git checkout 24e7ccb98951b0b4c7ae8a367273f8e73c074804
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=arm 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> drivers/gpu/drm/panel/panel-simple.c:185:32: error: field has incomplete type 'struct drm_edp_backlight_info'
           struct drm_edp_backlight_info info;
                                         ^
   drivers/gpu/drm/panel/panel-simple.c:185:9: note: forward declaration of 'struct drm_edp_backlight_info'
           struct drm_edp_backlight_info info;
                  ^
>> drivers/gpu/drm/panel/panel-simple.c:352:3: error: implicit declaration of function 'drm_edp_backlight_disable' [-Werror,-Wimplicit-function-declaration]
                   drm_edp_backlight_disable(p->aux, &bl->info);
                   ^
   drivers/gpu/drm/panel/panel-simple.c:352:3: note: did you mean 'backlight_disable'?
   include/linux/backlight.h:379:19: note: 'backlight_disable' declared here
   static inline int backlight_disable(struct backlight_device *bd)
                     ^
>> drivers/gpu/drm/panel/panel-simple.c:352:32: error: no member named 'aux' in 'struct panel_simple'
                   drm_edp_backlight_disable(p->aux, &bl->info);
                                             ~  ^
>> drivers/gpu/drm/panel/panel-simple.c:527:3: error: implicit declaration of function 'drm_edp_backlight_enable' [-Werror,-Wimplicit-function-declaration]
                   drm_edp_backlight_enable(p->aux, &bl->info,
                   ^
   drivers/gpu/drm/panel/panel-simple.c:527:3: note: did you mean 'backlight_enable'?
   include/linux/backlight.h:363:19: note: 'backlight_enable' declared here
   static inline int backlight_enable(struct backlight_device *bd)
                     ^
   drivers/gpu/drm/panel/panel-simple.c:527:31: error: no member named 'aux' in 'struct panel_simple'
                   drm_edp_backlight_enable(p->aux, &bl->info,
                                            ~  ^
>> drivers/gpu/drm/panel/panel-simple.c:598:9: error: implicit declaration of function 'drm_edp_backlight_set_level' [-Werror,-Wimplicit-function-declaration]
           return drm_edp_backlight_set_level(p->aux, &bl->info, bd->props.brightness);
                  ^
   drivers/gpu/drm/panel/panel-simple.c:598:40: error: no member named 'aux' in 'struct panel_simple'
           return drm_edp_backlight_set_level(p->aux, &bl->info, bd->props.brightness);
                                              ~  ^
>> drivers/gpu/drm/panel/panel-simple.c:611:14: error: use of undeclared identifier 'EDP_DISPLAY_CTL_CAP_SIZE'
           u8 edp_dpcd[EDP_DISPLAY_CTL_CAP_SIZE];
                       ^
>> drivers/gpu/drm/panel/panel-simple.c:618:8: error: implicit declaration of function 'drm_dp_dpcd_read' [-Werror,-Wimplicit-function-declaration]
           ret = drm_dp_dpcd_read(panel->aux, DP_EDP_DPCD_REV, edp_dpcd,
                 ^
   drivers/gpu/drm/panel/panel-simple.c:618:32: error: no member named 'aux' in 'struct panel_simple'
           ret = drm_dp_dpcd_read(panel->aux, DP_EDP_DPCD_REV, edp_dpcd,
                                  ~~~~~  ^
>> drivers/gpu/drm/panel/panel-simple.c:618:37: error: use of undeclared identifier 'DP_EDP_DPCD_REV'
           ret = drm_dp_dpcd_read(panel->aux, DP_EDP_DPCD_REV, edp_dpcd,
                                              ^
   drivers/gpu/drm/panel/panel-simple.c:619:11: error: use of undeclared identifier 'EDP_DISPLAY_CTL_CAP_SIZE'
                                  EDP_DISPLAY_CTL_CAP_SIZE);
                                  ^
>> drivers/gpu/drm/panel/panel-simple.c:623:8: error: implicit declaration of function 'drm_edp_backlight_init' [-Werror,-Wimplicit-function-declaration]
           ret = drm_edp_backlight_init(panel->aux, &bl->info, 0, edp_dpcd,
                 ^
   drivers/gpu/drm/panel/panel-simple.c:623:38: error: no member named 'aux' in 'struct panel_simple'
           ret = drm_edp_backlight_init(panel->aux, &bl->info, 0, edp_dpcd,
                                        ~~~~~  ^
   drivers/gpu/drm/panel/panel-simple.c:871:15: error: no member named 'aux' in 'struct panel_simple'
                   if (!panel->aux) {
                        ~~~~~  ^
   15 errors generated.


vim +185 drivers/gpu/drm/panel/panel-simple.c

   182	
   183	struct edp_backlight {
   184		struct backlight_device *dev;
 > 185		struct drm_edp_backlight_info info;
   186	};
   187	
   188	struct panel_simple {
   189		struct drm_panel base;
   190		bool enabled;
   191		bool no_hpd;
   192	
   193		bool prepared;
   194	
   195		ktime_t prepared_time;
   196		ktime_t unprepared_time;
   197	
   198		const struct panel_desc *desc;
   199	
   200		struct regulator *supply;
   201		struct i2c_adapter *ddc;
   202	
   203		struct gpio_desc *enable_gpio;
   204		struct gpio_desc *hpd_gpio;
   205	
   206		struct edid *edid;
   207	
   208		struct edp_backlight *edp_bl;
   209	
   210		struct drm_display_mode override_mode;
   211	
   212		enum drm_panel_orientation orientation;
   213	};
   214	
   215	static inline struct panel_simple *to_panel_simple(struct drm_panel *panel)
   216	{
   217		return container_of(panel, struct panel_simple, base);
   218	}
   219	
   220	static unsigned int panel_simple_get_timings_modes(struct panel_simple *panel,
   221							   struct drm_connector *connector)
   222	{
   223		struct drm_display_mode *mode;
   224		unsigned int i, num = 0;
   225	
   226		for (i = 0; i < panel->desc->num_timings; i++) {
   227			const struct display_timing *dt = &panel->desc->timings[i];
   228			struct videomode vm;
   229	
   230			videomode_from_timing(dt, &vm);
   231			mode = drm_mode_create(connector->dev);
   232			if (!mode) {
   233				dev_err(panel->base.dev, "failed to add mode %ux%u\n",
   234					dt->hactive.typ, dt->vactive.typ);
   235				continue;
   236			}
   237	
   238			drm_display_mode_from_videomode(&vm, mode);
   239	
   240			mode->type |= DRM_MODE_TYPE_DRIVER;
   241	
   242			if (panel->desc->num_timings == 1)
   243				mode->type |= DRM_MODE_TYPE_PREFERRED;
   244	
   245			drm_mode_probed_add(connector, mode);
   246			num++;
   247		}
   248	
   249		return num;
   250	}
   251	
   252	static unsigned int panel_simple_get_display_modes(struct panel_simple *panel,
   253							   struct drm_connector *connector)
   254	{
   255		struct drm_display_mode *mode;
   256		unsigned int i, num = 0;
   257	
   258		for (i = 0; i < panel->desc->num_modes; i++) {
   259			const struct drm_display_mode *m = &panel->desc->modes[i];
   260	
   261			mode = drm_mode_duplicate(connector->dev, m);
   262			if (!mode) {
   263				dev_err(panel->base.dev, "failed to add mode %ux%u@%u\n",
   264					m->hdisplay, m->vdisplay,
   265					drm_mode_vrefresh(m));
   266				continue;
   267			}
   268	
   269			mode->type |= DRM_MODE_TYPE_DRIVER;
   270	
   271			if (panel->desc->num_modes == 1)
   272				mode->type |= DRM_MODE_TYPE_PREFERRED;
   273	
   274			drm_mode_set_name(mode);
   275	
   276			drm_mode_probed_add(connector, mode);
   277			num++;
   278		}
   279	
   280		return num;
   281	}
   282	
   283	static int panel_simple_get_non_edid_modes(struct panel_simple *panel,
   284						   struct drm_connector *connector)
   285	{
   286		struct drm_display_mode *mode;
   287		bool has_override = panel->override_mode.type;
   288		unsigned int num = 0;
   289	
   290		if (!panel->desc)
   291			return 0;
   292	
   293		if (has_override) {
   294			mode = drm_mode_duplicate(connector->dev,
   295						  &panel->override_mode);
   296			if (mode) {
   297				drm_mode_probed_add(connector, mode);
   298				num = 1;
   299			} else {
   300				dev_err(panel->base.dev, "failed to add override mode\n");
   301			}
   302		}
   303	
   304		/* Only add timings if override was not there or failed to validate */
   305		if (num == 0 && panel->desc->num_timings)
   306			num = panel_simple_get_timings_modes(panel, connector);
   307	
   308		/*
   309		 * Only add fixed modes if timings/override added no mode.
   310		 *
   311		 * We should only ever have either the display timings specified
   312		 * or a fixed mode. Anything else is rather bogus.
   313		 */
   314		WARN_ON(panel->desc->num_timings && panel->desc->num_modes);
   315		if (num == 0)
   316			num = panel_simple_get_display_modes(panel, connector);
   317	
   318		connector->display_info.bpc = panel->desc->bpc;
   319		connector->display_info.width_mm = panel->desc->size.width;
   320		connector->display_info.height_mm = panel->desc->size.height;
   321		if (panel->desc->bus_format)
   322			drm_display_info_set_bus_formats(&connector->display_info,
   323							 &panel->desc->bus_format, 1);
   324		connector->display_info.bus_flags = panel->desc->bus_flags;
   325	
   326		return num;
   327	}
   328	
   329	static void panel_simple_wait(ktime_t start_ktime, unsigned int min_ms)
   330	{
   331		ktime_t now_ktime, min_ktime;
   332	
   333		if (!min_ms)
   334			return;
   335	
   336		min_ktime = ktime_add(start_ktime, ms_to_ktime(min_ms));
   337		now_ktime = ktime_get();
   338	
   339		if (ktime_before(now_ktime, min_ktime))
   340			msleep(ktime_to_ms(ktime_sub(min_ktime, now_ktime)) + 1);
   341	}
   342	
   343	static int panel_simple_disable(struct drm_panel *panel)
   344	{
   345		struct panel_simple *p = to_panel_simple(panel);
   346		struct edp_backlight *bl = p->edp_bl;
   347	
   348		if (!p->enabled)
   349			return 0;
   350	
   351		if (p->desc->uses_dpcd_backlight && bl)
 > 352			drm_edp_backlight_disable(p->aux, &bl->info);
   353	
   354		if (p->desc->delay.disable)
   355			msleep(p->desc->delay.disable);
   356	
   357		p->enabled = false;
   358	
   359		return 0;
   360	}
   361	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 37707 bytes --]

Re: [v4 1/4] drm/panel-simple: Add basic DPCD backlight support

[Doug Anderson]

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

Re: [v4 1/4] drm/panel-simple: Add basic DPCD backlight support

[rajeevny]

Hi,

On 25-05-2021 22:48, Doug Anderson wrote:
> 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 *".

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

> 
> 
>> +       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().

> 
>>         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.

Yes, this is not needed if the backlight is part of panel->backlight.

> 
> 
>>         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.

Will remove this too.

> 
> 
>>         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?
> 

The idea behind this check was to avoid the backlight update operation
(avoid DP aux access) when the panel is disabled. In case, if someone 
sets the
brightness from the sysfs when the panel is off. I should have used
backlight_get_brightness() or backlight_is_blank().

As we are moving this function out of the panel-simple, and going to use
panel->backlight, I will remove this check.

> 
>> +}
>> +
>> +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.
> 

This looks perfect. I will make the changes.

> 
>>  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 {"
Oops! I will fix this.

> 
> 
>> +               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.

Thanks, for the review.

-Rajeev

Re: [v4 1/4] drm/panel-simple: Add basic DPCD backlight support

[Doug Anderson]

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

Re: [v4 1/4] drm/panel-simple: Add basic DPCD backlight support

[Lyude Paul]

Sorry-I've been waiting to review this, but the DPCD backlight support helper
series is -still- blocked on getting reviews upstream :\

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.I18e60221f6d048d14d6c50a770b15f356fa75092@changeid/
> 
>  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);
>  

-- 
Cheers,
 Lyude Paul (she/her)
 Software Engineer at Red Hat

Re: [v4 1/4] drm/panel-simple: Add basic DPCD backlight support

[Lyude Paul]

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.I18e60221f6d048d14d6c50a770b15f356fa75092@changeid/
> 
>  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);
>  

-- 
Cheers,
 Lyude Paul (she/her)
 Software Engineer at Red Hat

Re: [v4 1/4] drm/panel-simple: Add basic DPCD backlight support

[rajeevny]

On 02-06-2021 03:50, Lyude Paul wrote:
> 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 :)
> 

Hi Lyude,

That's great!
I have updated v5 [1] of this series addressing Doug's review comments 
on v4 [2]. 
Please review the v5.

[1] 
https://lore.kernel.org/linux-arm-msm/1622390172-31368-1-git-send-email-rajeevny@codeaurora.org/
[2] 
https://lore.kernel.org/linux-arm-msm/CAD=FV=WzQ0Oc=e3kmNeBZUA+P1soKhBk8zt7bG1gqJ-Do-Tq_w@mail.gmail.com/


Thanks,
Rajeev

Re: [v4 1/4] drm/panel-simple: Add basic DPCD backlight support

[Doug Anderson]

Lyude,

On Tue, Jun 1, 2021 at 3:20 PM Lyude Paul <lyude@redhat.com> wrote:
>
> 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 :)

I'm going to assume that means that you don't need extra eyes on your
backlight patches. If you do, please shout and I'll try to find some
cycles for it. I've always got more things to do than there are hours
in the day, but many folks from the DRM community have helped me out
with numerous reviews over the last year or two and I'm happy to pay
some of that back by giving reviews if it'll help move things forward.
:-)

-Doug

[v4 2/4] drm/panel-simple: Support for delays between GPIO & regulator

[Rajeev Nandan]

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.
+		 */
+		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.
+		 */
+		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);
+
 	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);
+
 	gpiod_set_value_cansleep(p->enable_gpio, 1);
 
 	delay = p->desc->delay.prepare;
-- 
2.7.4

Re: [v4 2/4] drm/panel-simple: Support for delays between GPIO & regulator

[Doug Anderson]

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

[v4 3/4] dt-bindings: display: simple: Add Samsung ATNA33XC20

[Rajeev Nandan]

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
         # Samsung Electronics 10.1" WSVGA TFT LCD panel
-- 
2.7.4

Re: [v4 3/4] dt-bindings: display: simple: Add Samsung ATNA33XC20

[Doug Anderson]

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>

[v4 4/4] drm/panel-simple: Add Samsung ATNA33XC20

[Rajeev Nandan]

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,
+};
+
 static const struct drm_display_mode samsung_lsn122dl01_c01_mode = {
 	.clock = 271560,
 	.hdisplay = 2560,
@@ -4645,6 +4676,9 @@ static const struct of_device_id platform_of_match[] = {
 		.compatible = "rocktech,rk101ii01d-ct",
 		.data = &rocktech_rk101ii01d_ct,
 	}, {
+		.compatible = "samsung,atna33xc20",
+		.data = &samsung_atna33xc20,
+	}, {
 		.compatible = "samsung,lsn122dl01-c01",
 		.data = &samsung_lsn122dl01_c01,
 	}, {
-- 
2.7.4

Re: [v4 4/4] drm/panel-simple: Add Samsung ATNA33XC20

[Doug Anderson]

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.