[PATCH 03/13] drm/dp: Add argument for luminance range info in drm_edp_backlight_init

[Kandpal, Suraj]

> -----Original Message-----
> From: Murthy, Arun R <arun.r.murthy at intel.com>
> Sent: Thursday, June 19, 2025 10:47 AM
> To: Kandpal, Suraj <suraj.kandpal at intel.com>;
> nouveau at lists.freedesktop.org; dri-devel at lists.freedesktop.org; intel-
> xe at lists.freedesktop.org; intel-gfx at lists.freedesktop.org
> Cc: Nautiyal, Ankit K <ankit.k.nautiyal at intel.com>
> Subject: RE: [PATCH 03/13] drm/dp: Add argument for luminance range info in
> drm_edp_backlight_init
> 
> > > -----Original Message-----
> > > From: Murthy, Arun R <arun.r.murthy at intel.com>
> > > Sent: Thursday, June 12, 2025 4:43 PM
> > > To: Kandpal, Suraj <suraj.kandpal at intel.com>;
> > > nouveau at lists.freedesktop.org; dri-devel at lists.freedesktop.org;
> > > intel- xe at lists.freedesktop.org; intel-gfx at lists.freedesktop.org
> > > Cc: Nautiyal, Ankit K <ankit.k.nautiyal at intel.com>
> > > Subject: RE: [PATCH 03/13] drm/dp: Add argument for luminance range
> > > info in drm_edp_backlight_init
> > >
> > > > > > -----Original Message-----
> > > > > > From: Kandpal, Suraj <suraj.kandpal at intel.com>
> > > > > > Sent: Monday, April 14, 2025 9:46 AM
> > > > > > To: nouveau at lists.freedesktop.org;
> > > > > > dri-devel at lists.freedesktop.org;
> > > > > > intel- xe at lists.freedesktop.org;
> > > > > > intel-gfx at lists.freedesktop.org
> > > > > > Cc: Nautiyal, Ankit K <ankit.k.nautiyal at intel.com>; Murthy,
> > > > > > Arun R <arun.r.murthy at intel.com>; Kandpal, Suraj
> > > > > > <suraj.kandpal at intel.com>
> > > > > > Subject: [PATCH 03/13] drm/dp: Add argument for luminance
> > > > > > range info in drm_edp_backlight_init
> > > > > >
> > > > > > Add new argument to drm_edp_backlight_init which gives the
> > > > > > drm_luminance_range_info struct which will be needed to set
> > > > > > the min and max values for backlight.
> > > > > >
> > > > > > Signed-off-by: Suraj Kandpal <suraj.kandpal at intel.com>
> > > > > > ---
> > > > > >  drivers/gpu/drm/display/drm_dp_helper.c               | 5 ++++-
> > > > > >  drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c | 5 +++--
> > > > > >  drivers/gpu/drm/nouveau/nouveau_backlight.c           | 5 ++++-
> > > > > >  include/drm/display/drm_dp_helper.h                   | 1 +
> > > > > >  4 files changed, 12 insertions(+), 4 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/gpu/drm/display/drm_dp_helper.c
> > > > > > b/drivers/gpu/drm/display/drm_dp_helper.c
> > > > > > index 99b27e5e3365..3b309ac5190b 100644
> > > > > > --- a/drivers/gpu/drm/display/drm_dp_helper.c
> > > > > > +++ b/drivers/gpu/drm/display/drm_dp_helper.c
> > > > > > @@ -4227,6 +4227,8 @@ drm_edp_backlight_probe_state(struct
> > > > > drm_dp_aux
> > > > > > *aux, struct drm_edp_backlight_i
> > > > > >   * interface.
> > > > > >   * @aux: The DP aux device to use for probing
> > > > > >   * @bl: The &drm_edp_backlight_info struct to fill out with
> > > > > > information on the backlight
> > > > > > + * @lr: The &drm_luminance_range_info struct which is used to
> > > > > > + get the min max when using *luminance override
> > > > > >   * @driver_pwm_freq_hz: Optional PWM frequency from the
> > > > > > driver in
> > > hz
> > > > > >   * @edp_dpcd: A cached copy of the eDP DPCD
> > > > > >   * @current_level: Where to store the probed brightness
> > > > > > level, if any @@ -
> > > > > > 4243,6 +4245,7 @@ drm_edp_backlight_probe_state(struct
> > > drm_dp_aux
> > > > > > *aux, struct drm_edp_backlight_i
> > > > > >   */
> > > > > >  int
> > > > > >  drm_edp_backlight_init(struct drm_dp_aux *aux, struct
> > > > > > drm_edp_backlight_info *bl,
> > > > > > +		       struct drm_luminance_range_info *lr,
> > > > > Would it be better to have this drm_luminance_range_info inside
> > > > > the drm_edp_backlight_info?
> > > >
> > > > The thing is we fill drm_edp_backlight_info struct in
> > > > drm_edp_backlight_init Which means we would have to pass it
> anyways.
> > > > So having a reference of this in drm_edp_backlight_info didn't make
> sense.
> > > >
> > > The main intention for this ask is two xx_info struct passed as argument.
> > > Moreover luminance is part of backlight and this new element is
> > > _info and there already exists backlight_info. So wondering is
> > > luminance can be put inside backlight_info. The caller of this
> > > function can fill the luminance part and then make a call.
> > >
> >
> > I see you point but the thing is luminance range is not something we
> > will be using later and is only used the set the max level of brightness that
> can be set.
> > That being said I do get your point on sending two xx_info struct
> > here, I was thinking we send only the
> > U32 max luminance here since that's the only one we actually use.
> > Drivers can send the max luminance they like.
> > What do you think?
> >
> That should be better! 4th patch can be squashed with this one.
> 

Sure will do that

Regards,
Suraj Kandpal

> Thanks and Regards,
> Arun R Murthy
> --------------------
> > Regards,
> > Suraj Kandpal
> >
> > > Thanks and Regards,
> > > Arun R Murthy
> > > --------------------