linux-arm-msm.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: rajeevny@codeaurora.org
To: Doug Anderson <dianders@chromium.org>
Cc: dri-devel <dri-devel@lists.freedesktop.org>,
	linux-arm-msm <linux-arm-msm@vger.kernel.org>,
	freedreno <freedreno@lists.freedesktop.org>,
	"open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
	<devicetree@vger.kernel.org>, LKML <linux-kernel@vger.kernel.org>,
	Thierry Reding <thierry.reding@gmail.com>,
	Sam Ravnborg <sam@ravnborg.org>, Rob Clark <robdclark@gmail.com>,
	Lyude Paul <lyude@redhat.com>,
	Jani Nikula <jani.nikula@intel.com>,
	Rob Herring <robh@kernel.org>,
	Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
	Andrzej Hajda <a.hajda@samsung.com>,
	Daniel Thompson <daniel.thompson@linaro.org>,
	"Kristian H. Kristensen" <hoegsberg@chromium.org>,
	Abhinav Kumar <abhinavk@codeaurora.org>,
	Sean Paul <seanpaul@chromium.org>,
	Kalyan Thota <kalyan_t@codeaurora.org>,
	mkrishn@codeaurora.org
Subject: Re: [v4 1/4] drm/panel-simple: Add basic DPCD backlight support
Date: Thu, 27 May 2021 17:51:15 +0530	[thread overview]
Message-ID: <42db3a26684a5329287d57e1e78d0475@codeaurora.org> (raw)
In-Reply-To: <CAD=FV=WzQ0Oc=e3kmNeBZUA+P1soKhBk8zt7bG1gqJ-Do-Tq_w@mail.gmail.com>

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

  reply	other threads:[~2021-05-27 12:21 UTC|newest]

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-05-25  7:30 [v4 0/4] drm: Support basic DPCD backlight in panel-simple and add a new panel ATNA33XC20 Rajeev Nandan
2021-05-25  7:30 ` [v4 1/4] drm/panel-simple: Add basic DPCD backlight support Rajeev Nandan
2021-05-25 11:41   ` kernel test robot
2021-05-25 17:18   ` Doug Anderson
2021-05-27 12:21     ` rajeevny [this message]
2021-05-27 21:41       ` Doug Anderson
2021-06-01 18:28   ` Lyude Paul
2021-06-01 22:20   ` Lyude Paul
2021-06-02  5:38     ` rajeevny
2021-06-08 21:02     ` Doug Anderson
2021-05-25  7:30 ` [v4 2/4] drm/panel-simple: Support for delays between GPIO & regulator Rajeev Nandan
2021-05-25 17:18   ` Doug Anderson
2021-05-25  7:30 ` [v4 3/4] dt-bindings: display: simple: Add Samsung ATNA33XC20 Rajeev Nandan
2021-05-25 17:19   ` Doug Anderson
2021-05-25  7:30 ` [v4 4/4] drm/panel-simple: " Rajeev Nandan
2021-05-25 17:19   ` Doug Anderson

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=42db3a26684a5329287d57e1e78d0475@codeaurora.org \
    --to=rajeevny@codeaurora.org \
    --cc=a.hajda@samsung.com \
    --cc=abhinavk@codeaurora.org \
    --cc=daniel.thompson@linaro.org \
    --cc=devicetree@vger.kernel.org \
    --cc=dianders@chromium.org \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=freedreno@lists.freedesktop.org \
    --cc=hoegsberg@chromium.org \
    --cc=jani.nikula@intel.com \
    --cc=kalyan_t@codeaurora.org \
    --cc=laurent.pinchart@ideasonboard.com \
    --cc=linux-arm-msm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=lyude@redhat.com \
    --cc=mkrishn@codeaurora.org \
    --cc=robdclark@gmail.com \
    --cc=robh@kernel.org \
    --cc=sam@ravnborg.org \
    --cc=seanpaul@chromium.org \
    --cc=thierry.reding@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).