On Thu, 2020-11-26 at 11:03 +1000, Dave Airlie wrote:
> On Thu, 17 Sept 2020 at 03:19, Lyude Paul <lyude@xxxxxxxxxx> 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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
