Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Message-ID: <cb55ab8e5eee5ccece8212fa0576de16cc12dcd5.camel@redhat.com>
Subject: Re: [Intel-gfx] [RFC v2 3/8] drm/i915: Keep track of pwm-related
 backlight hooks separately
From:   Lyude Paul <lyude@redhat.com>
To:     Dave Airlie <airlied@gmail.com>
Cc:     Intel Graphics Development <intel-gfx@lists.freedesktop.org>,
        Lucas De Marchi <lucas.demarchi@intel.com>,
        David Airlie <airlied@linux.ie>,
        open list <linux-kernel@vger.kernel.org>,
        Chris Wilson <chris@chris-wilson.co.uk>,
        Vasily Khoruzhick <anarsoul@gmail.com>,
        Sean Paul <seanpaul@chromium.org>,
        dri-devel <dri-devel@lists.freedesktop.org>,
        Wambui Karuga <wambui.karugax@gmail.com>
Date:   Mon, 30 Nov 2020 18:19:02 -0500
In-Reply-To: <CAPM=9tzLK3caEW661tCeSPhn0G0NihuztCdgyABDgtvZRvV4xA@mail.gmail.com>
References: <20200916171855.129511-1-lyude@redhat.com>
         <20200916171855.129511-4-lyude@redhat.com>
         <CAPM=9tzLK3caEW661tCeSPhn0G0NihuztCdgyABDgtvZRvV4xA@mail.gmail.com>
Organization: Red Hat
Content-Type: text/plain; charset="UTF-8"
User-Agent: Evolution 3.38.1 (3.38.1-1.fc33) 
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Precedence: bulk

On Thu, 2020-11-26 at 11:03 +1000, Dave Airlie wrote:
> On Thu, 17 Sept 2020 at 03:19, Lyude Paul <lyude@redhat.com> wrote:
> > 
> > Currently, every different type of backlight hook that i915 supports is
> > pretty straight forward - you have a backlight, probably through PWM
> > (but maybe DPCD), with a single set of platform-specific hooks that are
> > used for controlling it.
> > 
> > HDR backlights, in particular VESA and Intel's HDR backlight
> > implementations, can end up being more complicated. With Intel's
> > proprietary interface, HDR backlight controls always run through the
> > DPCD. When the backlight is in SDR backlight mode however, the driver
> > may need to bypass the TCON and control the backlight directly through
> > PWM.
> > 
> > So, in order to support this we'll need to split our backlight callbacks
> > into two groups: a set of high-level backlight control callbacks in
> > intel_panel, and an additional set of pwm-specific backlight control
> > callbacks. This also implies a functional changes for how these
> > callbacks are used:
> > 
> > * We now keep track of two separate backlight level ranges, one for the
> >   high-level backlight, and one for the pwm backlight range
> > * We also keep track of backlight enablement and PWM backlight
> >   enablement separately
> > * Since the currently set backlight level might not be the same as the
> >   currently programmed PWM backlight level, we stop setting
> >   panel->backlight.level with the currently programmed PWM backlight
> >   level in panel->backlight.pwm_funcs.setup(). Instead, we rely
> >   on the higher level backlight control functions to retrieve the
> >   current PWM backlight level (in this case, intel_pwm_get_backlight()).
> >   Note that there are still a few PWM backlight setup callbacks that
> >   do actually need to retrieve the current PWM backlight level, although
> >   we no longer save this value in panel->backlight.level like before.
> > * panel->backlight.pwm_funcs.enable()/disable() both accept a PWM
> >   brightness level, unlike their siblings
> >   panel->backlight.enable()/disable(). This is so we can calculate the
> >   actual PWM brightness level we want to set on disable/enable in the
> >   higher level backlight enable()/disable() functions, since this value
> >   might be scaled from a brightness level that doesn't come from PWM.
> 
> Oh this patch is a handful, I can see why people stall out here.
> 
> I'm going to be annoying maintainer and see if you can clean this up a
> bit in advance
> of this patch.
> 

Not annoying at all :), I was hoping there'd be a good bit of criticism on
this patch series since it's been hard to figure out if I'm even implementing
things in the right way or not (especially because I really don't know what
the HDR side of this is going to look like, although I assume it's probably
going to be pretty hands-off in the kernel).

JFYI too for folks on the list, any suggestions about the HDR side of this are
super appreciated. I'm barely familiar with such things.

> 1) move the callbacks out of struct intel_panel.backlight into a separate
> struct
> and use const static object tables, having fn ptrs and data co-located
> in a struct
> isn't great.
> 
> strcut intel_panel_backlight_funcs {
> 
> };
> struct intel_panel {
>     struct {
>         struct intel_panel_backlight_funcs *funcs;
>     };
> };
> 
> type of thing.
> 
> I think you could reuse the backlight funcs struct for the pwm stuff
> as well. (maybe with an assert on hz_to_pwm for the old hooks).
> 
> 2) change the apis to pass 0 down in a separate patch, this modifies a
> bunch of apis to pass in an extra level parameter, do that
> first in a separate patch that doesn't change anything but hands 0
> down the chain. Then switch over in another patch.
> 
> 3) One comment in passing below.
> > 
> > 
> > -       if (cpu_mode)
> > -               val = pch_get_backlight(connector);
> > -       else
> > -               val = lpt_get_backlight(connector);
> > -       val = intel_panel_compute_brightness(connector, val);
> > -       panel->backlight.level = clamp(val, panel->backlight.min,
> > -                                      panel->backlight.max);
> > 
> >         if (cpu_mode) {
> > +               val = intel_panel_sanitize_pwm_level(connector,
> > pch_get_backlight(connector));
> > +
> >                 drm_dbg_kms(&dev_priv->drm,
> >                             "CPU backlight register was enabled, switching
> > to PCH override\n");
> > 
> >                 /* Write converted CPU PWM value to PCH override register
> > */
> > -               lpt_set_backlight(connector->base.state, panel-
> > >backlight.level);
> > +               lpt_set_backlight(connector->base.state, val);
> >                 intel_de_write(dev_priv, BLC_PWM_PCH_CTL1,
> >                                pch_ctl1 | BLM_PCH_OVERRIDE_ENABLE);
> > 
> The change here confused me since it no longer calls lpt_get_backlight
> in this path, the commit msg might explain this, but it didn't explain
> is so I could figure out if that was a mistake or intentional.

Will address these in the next respin, thanks for the review!

> 
> Dave.
> 

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