Am 26.06.21 um 18:51 schrieb Rajeev Nandan:

Some panels support backlight control over DP AUX channel using VESA's standard backlight control interface. Using new DRM eDP backlight helpers, add support to create and register a backlight for those panels in drm_panel to simplify the panel drivers.

The panel driver with access to "struct drm_dp_aux" can create and register a backlight device using following code snippet in its probe() function:

err = drm_panel_dp_aux_backlight(panel, aux); if (err) return err;

Then drm_panel will handle backlight_(enable|disable) calls similar to the case when drm_panel_of_backlight() is used.

Currently, we are not supporting one feature where the source device can combine the backlight brightness levels set through DP AUX and the BL_PWM_DIM eDP connector pin. Since it's not required for the basic backlight controls, it can be added later.

Signed-off-by: Rajeev Nandan rajeevny@codeaurora.org Reviewed-by: Douglas Anderson dianders@chromium.org Reviewed-by: Lyude Paul lyude@redhat.com

---

Changes in v5:

• New

Changes in v6:

• Fixed ordering of memory allocation (Douglas)
• Updated word wrapping in a comment (Douglas)

Changes in v8:

• Now using backlight_is_blank() to get the backlight blank status (Sam Ravnborg)

  drivers/gpu/drm/drm_panel.c | 108 ++++++++++++++++++++++++++++++++++++++++++++ include/drm/drm_panel.h | 15 ++++-- 2 files changed, 119 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c index f634371..4fa1e3b 100644 --- a/drivers/gpu/drm/drm_panel.c +++ b/drivers/gpu/drm/drm_panel.c @@ -26,12 +26,20 @@ #include <linux/module.h>

#include <drm/drm_crtc.h> +#include <drm/drm_dp_helper.h> #include <drm/drm_panel.h> #include <drm/drm_print.h>

static DEFINE_MUTEX(panel_lock); static LIST_HEAD(panel_list);

+struct dp_aux_backlight {

• struct backlight_device *base;
• struct drm_dp_aux *aux;
• struct drm_edp_backlight_info info;
• bool enabled;

+};

• /**
  • DOC: drm panel

@@ -342,6 +350,106 @@ int drm_panel_of_backlight(struct drm_panel *panel) return 0; } EXPORT_SYMBOL(drm_panel_of_backlight);

+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 (!backlight_is_blank(bd)) {

• 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;
  

• }
  

• }
• return ret;

+}

+static const struct backlight_ops dp_aux_bl_ops = {

• .update_status = dp_aux_backlight_update_status,

+};

+/**

• • 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;
  

• ret = drm_dp_dpcd_read(aux, DP_EDP_DPCD_REV, edp_dpcd,

• 	       EDP_DISPLAY_CTL_CAP_SIZE);
  

This creates a cyclic dependency between drm_kms_helper-ko and drm.ko. drm_panel.c is in the latter, while drm_dp_dpcd_read() in drm_dp_helper.c is in the former. Please fix.

Best regards Thomas

• 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;
  

• }
• bl = devm_kzalloc(panel->dev, sizeof(*bl), GFP_KERNEL);
• if (!bl)

• return -ENOMEM;
  

• bl->aux = aux;
• ret = drm_edp_backlight_init(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->base = devm_backlight_device_register(panel->dev, "dp_aux_backlight",

• 				  panel->dev, bl,
  

• 				  &dp_aux_bl_ops, &props);
  

• if (IS_ERR(bl->base))

• return PTR_ERR(bl->base);
  

• panel->backlight = bl->base;
• return 0;

+} +EXPORT_SYMBOL(drm_panel_dp_aux_backlight); #endif

MODULE_AUTHOR("Thierry Reding treding@nvidia.com"); diff --git a/include/drm/drm_panel.h b/include/drm/drm_panel.h index 33605c3..3ebfaa6 100644 --- a/include/drm/drm_panel.h +++ b/include/drm/drm_panel.h @@ -32,6 +32,7 @@ struct backlight_device; struct device_node; struct drm_connector; struct drm_device; +struct drm_dp_aux; struct drm_panel; struct display_timing;

@@ -64,8 +65,8 @@ enum drm_panel_orientation;

• the panel. This is the job of the .unprepare() function.
• Backlight can be handled automatically if configured using

• • drm_panel_of_backlight(). Then the driver does not need to implement the
• • functionality to enable/disable backlight.

• • drm_panel_of_backlight() or drm_panel_dp_aux_backlight(). Then the driver
• • does not need to implement the functionality to enable/disable backlight.
  */ struct drm_panel_funcs { /**

@@ -144,8 +145,8 @@ struct drm_panel { * Backlight device, used to turn on backlight after the call * to enable(), and to turn off backlight before the call to * disable().

• * backlight is set by drm_panel_of_backlight() and drivers
  

• * shall not assign it.
  

• * backlight is set by drm_panel_of_backlight() or
  

• * drm_panel_dp_aux_backlight() and drivers shall not assign it.
  

  */ struct backlight_device *backlight;

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

• 			     struct drm_dp_aux *aux)
  

+{

• return 0;

+} #endif

#endif

Hi Laurent,

On 27-06-2021 23:48, Laurent Pinchart wrote:

Hi Rajeev,

On Sat, Jun 26, 2021 at 10:21:06PM +0530, Rajeev Nandan wrote:

...

Do not give a warning for the eDP panels if the "bus_format" is not specified, since most eDP panels can support more than one bus formats and this can be auto-detected. Also, update the check to include bpc=10 for the eDP panel.

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

Changes in v8:

• New patch, to address the review comments of Sam Ravnborg [1]

[1] https://lore.kernel.org/dri-devel/20210621184157.GB918146@ravnborg.org/

drivers/gpu/drm/panel/panel-simple.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c index 86e5a45..f966b562 100644 --- a/drivers/gpu/drm/panel/panel-simple.c +++ b/drivers/gpu/drm/panel/panel-simple.c @@ -772,10 +772,8 @@ static int panel_simple_probe(struct device *dev, const struct panel_desc *desc, desc->bpc != 8); break; case DRM_MODE_CONNECTOR_eDP:

• if (desc->bus_format == 0)
  

• 	dev_warn(dev, "Specify missing bus_format\n");
  

• if (desc->bpc != 6 && desc->bpc != 8)
  

• 	dev_warn(dev, "Expected bpc in {6,8} but got: %u\n", desc->bpc);
  

• if (desc->bpc != 6 && desc->bpc != 8 && desc->bpc != 10)
  

• 	dev_warn(dev, "Expected bpc in {6,8,10} but got: %u\n", 
  

desc->bpc);

You'll still get a warning is bpc == 0, is that intentional ?

This was not intentional, I missed considering bpc=0 case. As we are removing the warning for bus_format=0 then a similar thing can be done for the bpc=0 also. The bpc value should be a valid one if it is specified. Unlike the bus_format, bpc has few possible values that can be checked here along with 0. Please correct me if I misunderstood the concept. I will fix this.

Thanks, Rajeev

Hi,

On Mon, Jun 28, 2021 at 6:33 AM Laurent Pinchart laurent.pinchart@ideasonboard.com wrote:

Hi Rajeev,

On Mon, Jun 28, 2021 at 05:46:24PM +0530, rajeevny@codeaurora.org wrote:

...

On 27-06-2021 23:48, Laurent Pinchart wrote:

...

On Sat, Jun 26, 2021 at 10:21:06PM +0530, Rajeev Nandan wrote:

...

Do not give a warning for the eDP panels if the "bus_format" is not specified, since most eDP panels can support more than one bus formats and this can be auto-detected. Also, update the check to include bpc=10 for the eDP panel.

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

Changes in v8:

• New patch, to address the review comments of Sam Ravnborg [1]

[1] https://lore.kernel.org/dri-devel/20210621184157.GB918146@ravnborg.org/

drivers/gpu/drm/panel/panel-simple.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c index 86e5a45..f966b562 100644 --- a/drivers/gpu/drm/panel/panel-simple.c +++ b/drivers/gpu/drm/panel/panel-simple.c @@ -772,10 +772,8 @@ static int panel_simple_probe(struct device *dev, const struct panel_desc *desc, desc->bpc != 8); break; case DRM_MODE_CONNECTOR_eDP:

•      if (desc->bus_format == 0)
  

•              dev_warn(dev, "Specify missing bus_format\n");
  

•      if (desc->bpc != 6 && desc->bpc != 8)
  

•              dev_warn(dev, "Expected bpc in {6,8} but got: %u\n", desc->bpc);
  

•      if (desc->bpc != 6 && desc->bpc != 8 && desc->bpc != 10)
  

•              dev_warn(dev, "Expected bpc in {6,8,10} but got: %u\n", desc->bpc);
  

You'll still get a warning is bpc == 0, is that intentional ?

This was not intentional, I missed considering bpc=0 case. As we are removing the warning for bus_format=0 then a similar thing can be done for the bpc=0 also. The bpc value should be a valid one if it is specified. Unlike the bus_format, bpc has few possible values that can be checked here along with 0. Please correct me if I misunderstood the concept. I will fix this.

What's the point of specifying bpc if it's optional though ? Users of the panel will need to support the case where bpc is set to 0. Have you ensured that they all do ? Can they meaningfully use the bpc value if they need to be ready to support bpc == 0 ?

I must be missing something, but to me it seems like Rajeev's patch is fine as-is. From my reading of the code:

* Removes the warning if bus_format == 0. This is correct since I don't think specifying bus format for eDP panels makes lots of sense.

* Removes the warning if bpc == 10. This is correct since we've seen eDP panels with 10bpc.

* Keeps the warning if bpc == 0. IMO we can/should still require panels to specify their BPC. I guess I'm treating this as a "max BPC". I know that we use this field in the sn65dsi86 driver, so if it's OK for this to be 0 then we'll have to change that driver to handle it.

Does that sound right to you Laurent? So since I think Rajeev's patch is OK, I'm happy with:

Reviewed-by: Douglas Anderson dianders@chromium.org

Unless I missed something and this patch needs to change then it feels like Rajeev's patch series is in pretty good shape to land. I'm happy to commit it but since Sam made comments on the previous version I'd plan to wait a bit to make sure he has a chance for another look if he wants to. I've also only got 2 days left before I vanish for 1 week of vacation. ...so my plan is: * If Sam / Laurent come back before tomorrow and say they're happy then I'll commit. * If I hear nothing then I'll check back after my vacation. If someone else has committed then I'll be happy. If not and there has just been silence then I'll commit it myself.

Please yell if that's not OK. :-)

-Doug