[RFC v2 1/8] drm/i915/dp: Program source OUI on eDP panels

[RFC v2 1/8] drm/i915/dp: Program source OUI on eDP panels

[Lyude Paul]

Since we're about to start adding support for Intel's magic HDR
backlight interface over DPCD, we need to ensure we're properly
programming this field so that Intel specific sink services are exposed.
Otherwise, 0x300-0x3ff will just read zeroes.

We also take care not to reprogram the source OUI if it already matches
what we expect. This is just to be careful so that we don't accidentally
take the panel out of any backlight control modes we found it in.

v2:
* Add careful parameter to intel_edp_init_source_oui() to avoid
  re-writing the source OUI if it's already been set during driver
  initialization

Signed-off-by: Lyude Paul <lyude@redhat.com>
Cc: thaytan@noraisin.net
Cc: Vasily Khoruzhick <anarsoul@gmail.com>
---
 drivers/gpu/drm/i915/display/intel_dp.c | 33 +++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
index 4bd10456ad188..7db2b6a3cd52e 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -3424,6 +3424,29 @@ void intel_dp_sink_set_decompression_state(struct intel_dp *intel_dp,
 			    enable ? "enable" : "disable");
 }
 
+static void
+intel_edp_init_source_oui(struct intel_dp *intel_dp, bool careful)
+{
+	struct drm_i915_private *i915 = dp_to_i915(intel_dp);
+	u8 oui[] = { 0x00, 0xaa, 0x01 };
+	u8 buf[3] = { 0 };
+
+	/*
+	 * During driver init, we want to be careful and avoid changing the source OUI if it's
+	 * already set to what we want, so as to avoid clearing any state by accident
+	 */
+	if (careful) {
+		if (drm_dp_dpcd_read(&intel_dp->aux, DP_SOURCE_OUI, buf, sizeof(buf)) < 0)
+			drm_err(&i915->drm, "Failed to read source OUI\n");
+
+		if (memcmp(oui, buf, sizeof(oui)) == 0)
+			return;
+	}
+
+	if (drm_dp_dpcd_write(&intel_dp->aux, DP_SOURCE_OUI, oui, sizeof(oui)) < 0)
+		drm_err(&i915->drm, "Failed to write source OUI\n");
+}
+
 /* If the sink supports it, try to set the power state appropriately */
 void intel_dp_sink_dpms(struct intel_dp *intel_dp, int mode)
 {
@@ -3443,6 +3466,10 @@ void intel_dp_sink_dpms(struct intel_dp *intel_dp, int mode)
 	} else {
 		struct intel_lspcon *lspcon = dp_to_lspcon(intel_dp);
 
+		/* Write the source OUI as early as possible */
+		if (intel_dp_is_edp(intel_dp))
+			intel_edp_init_source_oui(intel_dp, false);
+
 		/*
 		 * When turning on, we need to retry for 1ms to give the sink
 		 * time to wake up.
@@ -4607,6 +4634,12 @@ intel_edp_init_dpcd(struct intel_dp *intel_dp)
 	if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv))
 		intel_dp_get_dsc_sink_cap(intel_dp);
 
+	/*
+	 * If needed, program our source OUI so we can make various Intel-specific AUX services
+	 * available (such as HDR backlight controls)
+	 */
+	intel_edp_init_source_oui(intel_dp, true);
+
 	return true;
 }
 
-- 
2.26.2

Re: [RFC v2 1/8] drm/i915/dp: Program source OUI on eDP panels

[Rodrigo Vivi]

On Wed, Sep 16, 2020 at 01:18:48PM -0400, Lyude Paul wrote:
> Since we're about to start adding support for Intel's magic HDR
> backlight interface over DPCD, we need to ensure we're properly
> programming this field so that Intel specific sink services are exposed.
> Otherwise, 0x300-0x3ff will just read zeroes.
> 
> We also take care not to reprogram the source OUI if it already matches
> what we expect. This is just to be careful so that we don't accidentally
> take the panel out of any backlight control modes we found it in.
> 
> v2:
> * Add careful parameter to intel_edp_init_source_oui() to avoid
>   re-writing the source OUI if it's already been set during driver
>   initialization
> 
> Signed-off-by: Lyude Paul <lyude@redhat.com>
> Cc: thaytan@noraisin.net
> Cc: Vasily Khoruzhick <anarsoul@gmail.com>
> ---
>  drivers/gpu/drm/i915/display/intel_dp.c | 33 +++++++++++++++++++++++++
>  1 file changed, 33 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> index 4bd10456ad188..7db2b6a3cd52e 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -3424,6 +3424,29 @@ void intel_dp_sink_set_decompression_state(struct intel_dp *intel_dp,
>  			    enable ? "enable" : "disable");
>  }
>  
> +static void
> +intel_edp_init_source_oui(struct intel_dp *intel_dp, bool careful)
> +{
> +	struct drm_i915_private *i915 = dp_to_i915(intel_dp);
> +	u8 oui[] = { 0x00, 0xaa, 0x01 };
> +	u8 buf[3] = { 0 };
> +
> +	/*
> +	 * During driver init, we want to be careful and avoid changing the source OUI if it's
> +	 * already set to what we want, so as to avoid clearing any state by accident
> +	 */
> +	if (careful) {

my first reaction here is why the problem described on the commit message doesn't
appear during the init, and setting it to the same shouldn't be a problem... but
yeap, I agree the risk of taking panel down is high... let's move with the careful approach


Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>


> +		if (drm_dp_dpcd_read(&intel_dp->aux, DP_SOURCE_OUI, buf, sizeof(buf)) < 0)
> +			drm_err(&i915->drm, "Failed to read source OUI\n");
> +
> +		if (memcmp(oui, buf, sizeof(oui)) == 0)
> +			return;
> +	}
> +
> +	if (drm_dp_dpcd_write(&intel_dp->aux, DP_SOURCE_OUI, oui, sizeof(oui)) < 0)
> +		drm_err(&i915->drm, "Failed to write source OUI\n");
> +}
> +
>  /* If the sink supports it, try to set the power state appropriately */
>  void intel_dp_sink_dpms(struct intel_dp *intel_dp, int mode)
>  {
> @@ -3443,6 +3466,10 @@ void intel_dp_sink_dpms(struct intel_dp *intel_dp, int mode)
>  	} else {
>  		struct intel_lspcon *lspcon = dp_to_lspcon(intel_dp);
>  
> +		/* Write the source OUI as early as possible */
> +		if (intel_dp_is_edp(intel_dp))
> +			intel_edp_init_source_oui(intel_dp, false);
> +
>  		/*
>  		 * When turning on, we need to retry for 1ms to give the sink
>  		 * time to wake up.
> @@ -4607,6 +4634,12 @@ intel_edp_init_dpcd(struct intel_dp *intel_dp)
>  	if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv))
>  		intel_dp_get_dsc_sink_cap(intel_dp);
>  
> +	/*
> +	 * If needed, program our source OUI so we can make various Intel-specific AUX services
> +	 * available (such as HDR backlight controls)
> +	 */
> +	intel_edp_init_source_oui(intel_dp, true);
> +
>  	return true;
>  }
>  
> -- 
> 2.26.2
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC v2 1/8] drm/i915/dp: Program source OUI on eDP panels

[Vasily Khoruzhick]

On Wed, Sep 16, 2020 at 10:19 AM Lyude Paul <lyude@redhat.com> wrote:
>
> Since we're about to start adding support for Intel's magic HDR
> backlight interface over DPCD, we need to ensure we're properly
> programming this field so that Intel specific sink services are exposed.
> Otherwise, 0x300-0x3ff will just read zeroes.
>
> We also take care not to reprogram the source OUI if it already matches
> what we expect. This is just to be careful so that we don't accidentally
> take the panel out of any backlight control modes we found it in.
>
> v2:
> * Add careful parameter to intel_edp_init_source_oui() to avoid
>   re-writing the source OUI if it's already been set during driver
>   initialization
>
> Signed-off-by: Lyude Paul <lyude@redhat.com>
> Cc: thaytan@noraisin.net
> Cc: Vasily Khoruzhick <anarsoul@gmail.com>

Hi Lyude,

Sorry for a late reply.

Whole series is:

Tested-by: Vasily Khoruzhick <anarsoul@gmail.com>

Regards,
Vasily

> ---
>  drivers/gpu/drm/i915/display/intel_dp.c | 33 +++++++++++++++++++++++++
>  1 file changed, 33 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> index 4bd10456ad188..7db2b6a3cd52e 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -3424,6 +3424,29 @@ void intel_dp_sink_set_decompression_state(struct intel_dp *intel_dp,
>                             enable ? "enable" : "disable");
>  }
>
> +static void
> +intel_edp_init_source_oui(struct intel_dp *intel_dp, bool careful)
> +{
> +       struct drm_i915_private *i915 = dp_to_i915(intel_dp);
> +       u8 oui[] = { 0x00, 0xaa, 0x01 };
> +       u8 buf[3] = { 0 };
> +
> +       /*
> +        * During driver init, we want to be careful and avoid changing the source OUI if it's
> +        * already set to what we want, so as to avoid clearing any state by accident
> +        */
> +       if (careful) {
> +               if (drm_dp_dpcd_read(&intel_dp->aux, DP_SOURCE_OUI, buf, sizeof(buf)) < 0)
> +                       drm_err(&i915->drm, "Failed to read source OUI\n");
> +
> +               if (memcmp(oui, buf, sizeof(oui)) == 0)
> +                       return;
> +       }
> +
> +       if (drm_dp_dpcd_write(&intel_dp->aux, DP_SOURCE_OUI, oui, sizeof(oui)) < 0)
> +               drm_err(&i915->drm, "Failed to write source OUI\n");
> +}
> +
>  /* If the sink supports it, try to set the power state appropriately */
>  void intel_dp_sink_dpms(struct intel_dp *intel_dp, int mode)
>  {
> @@ -3443,6 +3466,10 @@ void intel_dp_sink_dpms(struct intel_dp *intel_dp, int mode)
>         } else {
>                 struct intel_lspcon *lspcon = dp_to_lspcon(intel_dp);
>
> +               /* Write the source OUI as early as possible */
> +               if (intel_dp_is_edp(intel_dp))
> +                       intel_edp_init_source_oui(intel_dp, false);
> +
>                 /*
>                  * When turning on, we need to retry for 1ms to give the sink
>                  * time to wake up.
> @@ -4607,6 +4634,12 @@ intel_edp_init_dpcd(struct intel_dp *intel_dp)
>         if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv))
>                 intel_dp_get_dsc_sink_cap(intel_dp);
>
> +       /*
> +        * If needed, program our source OUI so we can make various Intel-specific AUX services
> +        * available (such as HDR backlight controls)
> +        */
> +       intel_edp_init_source_oui(intel_dp, true);
> +
>         return true;
>  }
>
> --
> 2.26.2
>

Re: [RFC v2 1/8] drm/i915/dp: Program source OUI on eDP panels

[Jani Nikula]

On Wed, 16 Sep 2020, Lyude Paul <lyude@redhat.com> wrote:
> Since we're about to start adding support for Intel's magic HDR
> backlight interface over DPCD, we need to ensure we're properly
> programming this field so that Intel specific sink services are exposed.
> Otherwise, 0x300-0x3ff will just read zeroes.
>
> We also take care not to reprogram the source OUI if it already matches
> what we expect. This is just to be careful so that we don't accidentally
> take the panel out of any backlight control modes we found it in.
>
> v2:
> * Add careful parameter to intel_edp_init_source_oui() to avoid
>   re-writing the source OUI if it's already been set during driver
>   initialization
>
> Signed-off-by: Lyude Paul <lyude@redhat.com>
> Cc: thaytan@noraisin.net
> Cc: Vasily Khoruzhick <anarsoul@gmail.com>
> ---
>  drivers/gpu/drm/i915/display/intel_dp.c | 33 +++++++++++++++++++++++++
>  1 file changed, 33 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> index 4bd10456ad188..7db2b6a3cd52e 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -3424,6 +3424,29 @@ void intel_dp_sink_set_decompression_state(struct intel_dp *intel_dp,
>  			    enable ? "enable" : "disable");
>  }
>  
> +static void
> +intel_edp_init_source_oui(struct intel_dp *intel_dp, bool careful)
> +{
> +	struct drm_i915_private *i915 = dp_to_i915(intel_dp);
> +	u8 oui[] = { 0x00, 0xaa, 0x01 };

Nitpick, could be static const.

> +	u8 buf[3] = { 0 };
> +
> +	/*
> +	 * During driver init, we want to be careful and avoid changing the source OUI if it's
> +	 * already set to what we want, so as to avoid clearing any state by accident
> +	 */

Did you actually observe any ill behaviour with unconditionally writing
the source OUI?

I mean it's easy to add the "careful" mode afterwards if there are
concrete issues, but it'll be hard to remove because it'll be a
functional change potentially causing regressions.

> +	if (careful) {
> +		if (drm_dp_dpcd_read(&intel_dp->aux, DP_SOURCE_OUI, buf, sizeof(buf)) < 0)
> +			drm_err(&i915->drm, "Failed to read source OUI\n");
> +
> +		if (memcmp(oui, buf, sizeof(oui)) == 0)
> +			return;
> +	}
> +
> +	if (drm_dp_dpcd_write(&intel_dp->aux, DP_SOURCE_OUI, oui, sizeof(oui)) < 0)
> +		drm_err(&i915->drm, "Failed to write source OUI\n");
> +}
> +
>  /* If the sink supports it, try to set the power state appropriately */
>  void intel_dp_sink_dpms(struct intel_dp *intel_dp, int mode)
>  {
> @@ -3443,6 +3466,10 @@ void intel_dp_sink_dpms(struct intel_dp *intel_dp, int mode)
>  	} else {
>  		struct intel_lspcon *lspcon = dp_to_lspcon(intel_dp);
>  
> +		/* Write the source OUI as early as possible */
> +		if (intel_dp_is_edp(intel_dp))
> +			intel_edp_init_source_oui(intel_dp, false);
> +

This hunk conflicts, will need a rebase.

BR,
Jani.


>  		/*
>  		 * When turning on, we need to retry for 1ms to give the sink
>  		 * time to wake up.
> @@ -4607,6 +4634,12 @@ intel_edp_init_dpcd(struct intel_dp *intel_dp)
>  	if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv))
>  		intel_dp_get_dsc_sink_cap(intel_dp);
>  
> +	/*
> +	 * If needed, program our source OUI so we can make various Intel-specific AUX services
> +	 * available (such as HDR backlight controls)
> +	 */
> +	intel_edp_init_source_oui(intel_dp, true);
> +
>  	return true;
>  }

-- 
Jani Nikula, Intel Open Source Graphics Center

Re: [RFC v2 1/8] drm/i915/dp: Program source OUI on eDP panels

[Lyude Paul]

On Thu, 2020-11-26 at 12:51 +0200, Jani Nikula wrote:
> On Wed, 16 Sep 2020, Lyude Paul <lyude@redhat.com> wrote:
> > Since we're about to start adding support for Intel's magic HDR
> > backlight interface over DPCD, we need to ensure we're properly
> > programming this field so that Intel specific sink services are exposed.
> > Otherwise, 0x300-0x3ff will just read zeroes.
> > 
> > We also take care not to reprogram the source OUI if it already matches
> > what we expect. This is just to be careful so that we don't accidentally
> > take the panel out of any backlight control modes we found it in.
> > 
> > v2:
> > * Add careful parameter to intel_edp_init_source_oui() to avoid
> >   re-writing the source OUI if it's already been set during driver
> >   initialization
> > 
> > Signed-off-by: Lyude Paul <lyude@redhat.com>
> > Cc: thaytan@noraisin.net
> > Cc: Vasily Khoruzhick <anarsoul@gmail.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_dp.c | 33 +++++++++++++++++++++++++
> >  1 file changed, 33 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
> > b/drivers/gpu/drm/i915/display/intel_dp.c
> > index 4bd10456ad188..7db2b6a3cd52e 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > @@ -3424,6 +3424,29 @@ void intel_dp_sink_set_decompression_state(struct
> > intel_dp *intel_dp,
> >                             enable ? "enable" : "disable");
> >  }
> >  
> > +static void
> > +intel_edp_init_source_oui(struct intel_dp *intel_dp, bool careful)
> > +{
> > +       struct drm_i915_private *i915 = dp_to_i915(intel_dp);
> > +       u8 oui[] = { 0x00, 0xaa, 0x01 };
> 
> Nitpick, could be static const.
> 
> > +       u8 buf[3] = { 0 };
> > +
> > +       /*
> > +        * During driver init, we want to be careful and avoid changing
> > the source OUI if it's
> > +        * already set to what we want, so as to avoid clearing any state
> > by accident
> > +        */
> 
> Did you actually observe any ill behaviour with unconditionally writing
> the source OUI?
> 
> I mean it's easy to add the "careful" mode afterwards if there are
> concrete issues, but it'll be hard to remove because it'll be a
> functional change potentially causing regressions.

I haven't yet, although I'll give a test on some of the other machines I've
got lying around.

FWIW, relevant spec info:

   A Sink device that does not support the Source-device specified behavior
   specified by the owner of the IEEE OUI written to in DPCD Addresses 00300h
   through 00302h as being associated with the Source Identification shall
   AUX_ACK all writes, but take no other action, and shall respond to reads
   with an AUX_ACK and the value 00h.

> 
> > +       if (careful) {
> > +               if (drm_dp_dpcd_read(&intel_dp->aux, DP_SOURCE_OUI, buf,
> > sizeof(buf)) < 0)
> > +                       drm_err(&i915->drm, "Failed to read source
> > OUI\n");
> > +
> > +               if (memcmp(oui, buf, sizeof(oui)) == 0)
> > +                       return;
> > +       }
> > +
> > +       if (drm_dp_dpcd_write(&intel_dp->aux, DP_SOURCE_OUI, oui,
> > sizeof(oui)) < 0)
> > +               drm_err(&i915->drm, "Failed to write source OUI\n");
> > +}
> > +
> >  /* If the sink supports it, try to set the power state appropriately */
> >  void intel_dp_sink_dpms(struct intel_dp *intel_dp, int mode)
> >  {
> > @@ -3443,6 +3466,10 @@ void intel_dp_sink_dpms(struct intel_dp *intel_dp,
> > int mode)
> >         } else {
> >                 struct intel_lspcon *lspcon = dp_to_lspcon(intel_dp);
> >  
> > +               /* Write the source OUI as early as possible */
> > +               if (intel_dp_is_edp(intel_dp))
> > +                       intel_edp_init_source_oui(intel_dp, false);
> > +
> 
> This hunk conflicts, will need a rebase.
> 
> BR,
> Jani.
> 
> 
> >                 /*
> >                  * When turning on, we need to retry for 1ms to give the
> > sink
> >                  * time to wake up.
> > @@ -4607,6 +4634,12 @@ intel_edp_init_dpcd(struct intel_dp *intel_dp)
> >         if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv))
> >                 intel_dp_get_dsc_sink_cap(intel_dp);
> >  
> > +       /*
> > +        * If needed, program our source OUI so we can make various Intel-
> > specific AUX services
> > +        * available (such as HDR backlight controls)
> > +        */
> > +       intel_edp_init_source_oui(intel_dp, true);
> > +
> >         return true;
> >  }
> 

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

[RFC v2 2/8] drm/i915: Rename pwm_* backlight callbacks to ext_pwm_*

[Lyude Paul]

Since we're going to need to add a set of lower-level PWM backlight
control hooks to be shared by normal backlight controls and HDR
backlight controls in SDR mode, let's add a prefix to the external PWM
backlight functions so that the difference between them and the high
level PWM-only backlight functions is a bit more obvious.

This introduces no functional changes.

Signed-off-by: Lyude Paul <lyude@redhat.com>
Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: thaytan@noraisin.net
Cc: Vasily Khoruzhick <anarsoul@gmail.com>
---
 drivers/gpu/drm/i915/display/intel_panel.c | 24 +++++++++++-----------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_panel.c b/drivers/gpu/drm/i915/display/intel_panel.c
index 9f23bac0d7924..c0e36244bb07d 100644
--- a/drivers/gpu/drm/i915/display/intel_panel.c
+++ b/drivers/gpu/drm/i915/display/intel_panel.c
@@ -589,7 +589,7 @@ static u32 bxt_get_backlight(struct intel_connector *connector)
 			     BXT_BLC_PWM_DUTY(panel->backlight.controller));
 }
 
-static u32 pwm_get_backlight(struct intel_connector *connector)
+static u32 ext_pwm_get_backlight(struct intel_connector *connector)
 {
 	struct intel_panel *panel = &connector->panel;
 	struct pwm_state state;
@@ -666,7 +666,7 @@ static void bxt_set_backlight(const struct drm_connector_state *conn_state, u32
 		       BXT_BLC_PWM_DUTY(panel->backlight.controller), level);
 }
 
-static void pwm_set_backlight(const struct drm_connector_state *conn_state, u32 level)
+static void ext_pwm_set_backlight(const struct drm_connector_state *conn_state, u32 level)
 {
 	struct intel_panel *panel = &to_intel_connector(conn_state->connector)->panel;
 
@@ -835,7 +835,7 @@ static void cnp_disable_backlight(const struct drm_connector_state *old_conn_sta
 		       tmp & ~BXT_BLC_PWM_ENABLE);
 }
 
-static void pwm_disable_backlight(const struct drm_connector_state *old_conn_state)
+static void ext_pwm_disable_backlight(const struct drm_connector_state *old_conn_state)
 {
 	struct intel_connector *connector = to_intel_connector(old_conn_state->connector);
 	struct intel_panel *panel = &connector->panel;
@@ -1168,8 +1168,8 @@ static void cnp_enable_backlight(const struct intel_crtc_state *crtc_state,
 		       pwm_ctl | BXT_BLC_PWM_ENABLE);
 }
 
-static void pwm_enable_backlight(const struct intel_crtc_state *crtc_state,
-				 const struct drm_connector_state *conn_state)
+static void ext_pwm_enable_backlight(const struct intel_crtc_state *crtc_state,
+				     const struct drm_connector_state *conn_state)
 {
 	struct intel_connector *connector = to_intel_connector(conn_state->connector);
 	struct intel_panel *panel = &connector->panel;
@@ -1890,8 +1890,8 @@ cnp_setup_backlight(struct intel_connector *connector, enum pipe unused)
 	return 0;
 }
 
-static int pwm_setup_backlight(struct intel_connector *connector,
-			       enum pipe pipe)
+static int ext_pwm_setup_backlight(struct intel_connector *connector,
+				   enum pipe pipe)
 {
 	struct drm_device *dev = connector->base.dev;
 	struct drm_i915_private *dev_priv = to_i915(dev);
@@ -2065,11 +2065,11 @@ intel_panel_init_backlight_funcs(struct intel_panel *panel)
 		panel->backlight.hz_to_pwm = pch_hz_to_pwm;
 	} else if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
 		if (connector->base.connector_type == DRM_MODE_CONNECTOR_DSI) {
-			panel->backlight.setup = pwm_setup_backlight;
-			panel->backlight.enable = pwm_enable_backlight;
-			panel->backlight.disable = pwm_disable_backlight;
-			panel->backlight.set = pwm_set_backlight;
-			panel->backlight.get = pwm_get_backlight;
+			panel->backlight.setup = ext_pwm_setup_backlight;
+			panel->backlight.enable = ext_pwm_enable_backlight;
+			panel->backlight.disable = ext_pwm_disable_backlight;
+			panel->backlight.set = ext_pwm_set_backlight;
+			panel->backlight.get = ext_pwm_get_backlight;
 		} else {
 			panel->backlight.setup = vlv_setup_backlight;
 			panel->backlight.enable = vlv_enable_backlight;
-- 
2.26.2

Re: [RFC v2 2/8] drm/i915: Rename pwm_* backlight callbacks to ext_pwm_*

[Jani Nikula]

On Wed, 16 Sep 2020, Lyude Paul <lyude@redhat.com> wrote:
> Since we're going to need to add a set of lower-level PWM backlight
> control hooks to be shared by normal backlight controls and HDR
> backlight controls in SDR mode, let's add a prefix to the external PWM
> backlight functions so that the difference between them and the high
> level PWM-only backlight functions is a bit more obvious.
>
> This introduces no functional changes.
>
> Signed-off-by: Lyude Paul <lyude@redhat.com>
> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: thaytan@noraisin.net
> Cc: Vasily Khoruzhick <anarsoul@gmail.com>

Reviewed-by: Jani Nikula <jani.nikula@intel.com>

> ---
>  drivers/gpu/drm/i915/display/intel_panel.c | 24 +++++++++++-----------
>  1 file changed, 12 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_panel.c b/drivers/gpu/drm/i915/display/intel_panel.c
> index 9f23bac0d7924..c0e36244bb07d 100644
> --- a/drivers/gpu/drm/i915/display/intel_panel.c
> +++ b/drivers/gpu/drm/i915/display/intel_panel.c
> @@ -589,7 +589,7 @@ static u32 bxt_get_backlight(struct intel_connector *connector)
>  			     BXT_BLC_PWM_DUTY(panel->backlight.controller));
>  }
>  
> -static u32 pwm_get_backlight(struct intel_connector *connector)
> +static u32 ext_pwm_get_backlight(struct intel_connector *connector)
>  {
>  	struct intel_panel *panel = &connector->panel;
>  	struct pwm_state state;
> @@ -666,7 +666,7 @@ static void bxt_set_backlight(const struct drm_connector_state *conn_state, u32
>  		       BXT_BLC_PWM_DUTY(panel->backlight.controller), level);
>  }
>  
> -static void pwm_set_backlight(const struct drm_connector_state *conn_state, u32 level)
> +static void ext_pwm_set_backlight(const struct drm_connector_state *conn_state, u32 level)
>  {
>  	struct intel_panel *panel = &to_intel_connector(conn_state->connector)->panel;
>  
> @@ -835,7 +835,7 @@ static void cnp_disable_backlight(const struct drm_connector_state *old_conn_sta
>  		       tmp & ~BXT_BLC_PWM_ENABLE);
>  }
>  
> -static void pwm_disable_backlight(const struct drm_connector_state *old_conn_state)
> +static void ext_pwm_disable_backlight(const struct drm_connector_state *old_conn_state)
>  {
>  	struct intel_connector *connector = to_intel_connector(old_conn_state->connector);
>  	struct intel_panel *panel = &connector->panel;
> @@ -1168,8 +1168,8 @@ static void cnp_enable_backlight(const struct intel_crtc_state *crtc_state,
>  		       pwm_ctl | BXT_BLC_PWM_ENABLE);
>  }
>  
> -static void pwm_enable_backlight(const struct intel_crtc_state *crtc_state,
> -				 const struct drm_connector_state *conn_state)
> +static void ext_pwm_enable_backlight(const struct intel_crtc_state *crtc_state,
> +				     const struct drm_connector_state *conn_state)
>  {
>  	struct intel_connector *connector = to_intel_connector(conn_state->connector);
>  	struct intel_panel *panel = &connector->panel;
> @@ -1890,8 +1890,8 @@ cnp_setup_backlight(struct intel_connector *connector, enum pipe unused)
>  	return 0;
>  }
>  
> -static int pwm_setup_backlight(struct intel_connector *connector,
> -			       enum pipe pipe)
> +static int ext_pwm_setup_backlight(struct intel_connector *connector,
> +				   enum pipe pipe)
>  {
>  	struct drm_device *dev = connector->base.dev;
>  	struct drm_i915_private *dev_priv = to_i915(dev);
> @@ -2065,11 +2065,11 @@ intel_panel_init_backlight_funcs(struct intel_panel *panel)
>  		panel->backlight.hz_to_pwm = pch_hz_to_pwm;
>  	} else if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
>  		if (connector->base.connector_type == DRM_MODE_CONNECTOR_DSI) {
> -			panel->backlight.setup = pwm_setup_backlight;
> -			panel->backlight.enable = pwm_enable_backlight;
> -			panel->backlight.disable = pwm_disable_backlight;
> -			panel->backlight.set = pwm_set_backlight;
> -			panel->backlight.get = pwm_get_backlight;
> +			panel->backlight.setup = ext_pwm_setup_backlight;
> +			panel->backlight.enable = ext_pwm_enable_backlight;
> +			panel->backlight.disable = ext_pwm_disable_backlight;
> +			panel->backlight.set = ext_pwm_set_backlight;
> +			panel->backlight.get = ext_pwm_get_backlight;
>  		} else {
>  			panel->backlight.setup = vlv_setup_backlight;
>  			panel->backlight.enable = vlv_enable_backlight;

-- 
Jani Nikula, Intel Open Source Graphics Center

[RFC v2 3/8] drm/i915: Keep track of pwm-related backlight hooks separately

[Lyude Paul]

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.

Signed-off-by: Lyude Paul <lyude@redhat.com>
Cc: thaytan@noraisin.net
Cc: Vasily Khoruzhick <anarsoul@gmail.com>
---
 .../drm/i915/display/intel_display_types.h    |  14 +-
 drivers/gpu/drm/i915/display/intel_panel.c    | 436 ++++++++++--------
 2 files changed, 246 insertions(+), 204 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
index b2d0edacc58c9..52a6543df842a 100644
--- a/drivers/gpu/drm/i915/display/intel_display_types.h
+++ b/drivers/gpu/drm/i915/display/intel_display_types.h
@@ -221,6 +221,9 @@ struct intel_panel {
 		bool alternate_pwm_increment;	/* lpt+ */
 
 		/* PWM chip */
+		u32 pwm_min;
+		u32 pwm_max;
+		bool pwm_enabled;
 		bool util_pin_active_low;	/* bxt+ */
 		u8 controller;		/* bxt+ only */
 		struct pwm_device *pwm;
@@ -229,6 +232,16 @@ struct intel_panel {
 		/* DPCD backlight */
 		u8 pwmgen_bit_count;
 
+		struct {
+			int (*setup)(struct intel_connector *connector, enum pipe pipe);
+			u32 (*get)(struct intel_connector *connector);
+			void (*set)(const struct drm_connector_state *conn_state, u32 level);
+			void (*disable)(const struct drm_connector_state *conn_state, u32 level);
+			void (*enable)(const struct intel_crtc_state *crtc_state,
+				       const struct drm_connector_state *conn_state, u32 level);
+			u32 (*hz_to_pwm)(struct intel_connector *connector, u32 hz);
+		} pwm_funcs;
+
 		struct backlight_device *device;
 
 		/* Connector and platform specific backlight functions */
@@ -238,7 +251,6 @@ struct intel_panel {
 		void (*disable)(const struct drm_connector_state *conn_state);
 		void (*enable)(const struct intel_crtc_state *crtc_state,
 			       const struct drm_connector_state *conn_state);
-		u32 (*hz_to_pwm)(struct intel_connector *connector, u32 hz);
 		void (*power)(struct intel_connector *, bool enable);
 	} backlight;
 };
diff --git a/drivers/gpu/drm/i915/display/intel_panel.c b/drivers/gpu/drm/i915/display/intel_panel.c
index c0e36244bb07d..6d3e9d51d069c 100644
--- a/drivers/gpu/drm/i915/display/intel_panel.c
+++ b/drivers/gpu/drm/i915/display/intel_panel.c
@@ -511,25 +511,34 @@ static u32 scale_hw_to_user(struct intel_connector *connector,
 		     0, user_max);
 }
 
-static u32 intel_panel_compute_brightness(struct intel_connector *connector,
-					  u32 val)
+static u32 intel_panel_sanitize_pwm_level(struct intel_connector *connector, u32 val)
 {
 	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
 	struct intel_panel *panel = &connector->panel;
 
-	drm_WARN_ON(&dev_priv->drm, panel->backlight.max == 0);
+	drm_WARN_ON(&dev_priv->drm, panel->backlight.pwm_max == 0);
 
 	if (dev_priv->params.invert_brightness < 0)
 		return val;
 
 	if (dev_priv->params.invert_brightness > 0 ||
 	    dev_priv->quirks & QUIRK_INVERT_BRIGHTNESS) {
-		return panel->backlight.max - val + panel->backlight.min;
+		return panel->backlight.pwm_max - val + panel->backlight.pwm_min;
 	}
 
 	return val;
 }
 
+static void intel_panel_set_pwm_level(const struct drm_connector_state *conn_state, u32 val)
+{
+	struct intel_connector *connector = to_intel_connector(conn_state->connector);
+	struct drm_i915_private *i915 = to_i915(connector->base.dev);
+	struct intel_panel *panel = &connector->panel;
+
+	drm_dbg_kms(&i915->drm, "set backlight PWM = %d\n", val);
+	panel->backlight.pwm_funcs.set(conn_state, val);
+}
+
 static u32 lpt_get_backlight(struct intel_connector *connector)
 {
 	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
@@ -624,12 +633,12 @@ static void i9xx_set_backlight(const struct drm_connector_state *conn_state, u32
 	struct intel_panel *panel = &connector->panel;
 	u32 tmp, mask;
 
-	drm_WARN_ON(&dev_priv->drm, panel->backlight.max == 0);
+	drm_WARN_ON(&dev_priv->drm, panel->backlight.pwm_max == 0);
 
 	if (panel->backlight.combination_mode) {
 		u8 lbpc;
 
-		lbpc = level * 0xfe / panel->backlight.max + 1;
+		lbpc = level * 0xfe / panel->backlight.pwm_max + 1;
 		level /= lbpc;
 		pci_write_config_byte(dev_priv->drm.pdev, LBPC, lbpc);
 	}
@@ -681,9 +690,8 @@ intel_panel_actually_set_backlight(const struct drm_connector_state *conn_state,
 	struct drm_i915_private *i915 = to_i915(connector->base.dev);
 	struct intel_panel *panel = &connector->panel;
 
-	drm_dbg_kms(&i915->drm, "set backlight PWM = %d\n", level);
+	drm_dbg_kms(&i915->drm, "set backlight level = %d\n", level);
 
-	level = intel_panel_compute_brightness(connector, level);
 	panel->backlight.set(conn_state, level);
 }
 
@@ -726,13 +734,13 @@ void intel_panel_set_backlight_acpi(const struct drm_connector_state *conn_state
 	mutex_unlock(&dev_priv->backlight_lock);
 }
 
-static void lpt_disable_backlight(const struct drm_connector_state *old_conn_state)
+static void lpt_disable_backlight(const struct drm_connector_state *old_conn_state, u32 level)
 {
 	struct intel_connector *connector = to_intel_connector(old_conn_state->connector);
 	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
 	u32 tmp;
 
-	intel_panel_actually_set_backlight(old_conn_state, 0);
+	intel_panel_set_pwm_level(old_conn_state, level);
 
 	/*
 	 * Although we don't support or enable CPU PWM with LPT/SPT based
@@ -754,13 +762,13 @@ static void lpt_disable_backlight(const struct drm_connector_state *old_conn_sta
 	intel_de_write(dev_priv, BLC_PWM_PCH_CTL1, tmp & ~BLM_PCH_PWM_ENABLE);
 }
 
-static void pch_disable_backlight(const struct drm_connector_state *old_conn_state)
+static void pch_disable_backlight(const struct drm_connector_state *old_conn_state, u32 val)
 {
 	struct intel_connector *connector = to_intel_connector(old_conn_state->connector);
 	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
 	u32 tmp;
 
-	intel_panel_actually_set_backlight(old_conn_state, 0);
+	intel_panel_set_pwm_level(old_conn_state, val);
 
 	tmp = intel_de_read(dev_priv, BLC_PWM_CPU_CTL2);
 	intel_de_write(dev_priv, BLC_PWM_CPU_CTL2, tmp & ~BLM_PWM_ENABLE);
@@ -769,44 +777,44 @@ static void pch_disable_backlight(const struct drm_connector_state *old_conn_sta
 	intel_de_write(dev_priv, BLC_PWM_PCH_CTL1, tmp & ~BLM_PCH_PWM_ENABLE);
 }
 
-static void i9xx_disable_backlight(const struct drm_connector_state *old_conn_state)
+static void i9xx_disable_backlight(const struct drm_connector_state *old_conn_state, u32 val)
 {
-	intel_panel_actually_set_backlight(old_conn_state, 0);
+	intel_panel_set_pwm_level(old_conn_state, val);
 }
 
-static void i965_disable_backlight(const struct drm_connector_state *old_conn_state)
+static void i965_disable_backlight(const struct drm_connector_state *old_conn_state, u32 val)
 {
 	struct drm_i915_private *dev_priv = to_i915(old_conn_state->connector->dev);
 	u32 tmp;
 
-	intel_panel_actually_set_backlight(old_conn_state, 0);
+	intel_panel_set_pwm_level(old_conn_state, val);
 
 	tmp = intel_de_read(dev_priv, BLC_PWM_CTL2);
 	intel_de_write(dev_priv, BLC_PWM_CTL2, tmp & ~BLM_PWM_ENABLE);
 }
 
-static void vlv_disable_backlight(const struct drm_connector_state *old_conn_state)
+static void vlv_disable_backlight(const struct drm_connector_state *old_conn_state, u32 val)
 {
 	struct intel_connector *connector = to_intel_connector(old_conn_state->connector);
 	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
 	enum pipe pipe = to_intel_crtc(old_conn_state->crtc)->pipe;
 	u32 tmp;
 
-	intel_panel_actually_set_backlight(old_conn_state, 0);
+	intel_panel_set_pwm_level(old_conn_state, val);
 
 	tmp = intel_de_read(dev_priv, VLV_BLC_PWM_CTL2(pipe));
 	intel_de_write(dev_priv, VLV_BLC_PWM_CTL2(pipe),
 		       tmp & ~BLM_PWM_ENABLE);
 }
 
-static void bxt_disable_backlight(const struct drm_connector_state *old_conn_state)
+static void bxt_disable_backlight(const struct drm_connector_state *old_conn_state, u32 val)
 {
 	struct intel_connector *connector = to_intel_connector(old_conn_state->connector);
 	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
 	struct intel_panel *panel = &connector->panel;
-	u32 tmp, val;
+	u32 tmp;
 
-	intel_panel_actually_set_backlight(old_conn_state, 0);
+	intel_panel_set_pwm_level(old_conn_state, val);
 
 	tmp = intel_de_read(dev_priv,
 			    BXT_BLC_PWM_CTL(panel->backlight.controller));
@@ -820,14 +828,14 @@ static void bxt_disable_backlight(const struct drm_connector_state *old_conn_sta
 	}
 }
 
-static void cnp_disable_backlight(const struct drm_connector_state *old_conn_state)
+static void cnp_disable_backlight(const struct drm_connector_state *old_conn_state, u32 val)
 {
 	struct intel_connector *connector = to_intel_connector(old_conn_state->connector);
 	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
 	struct intel_panel *panel = &connector->panel;
 	u32 tmp;
 
-	intel_panel_actually_set_backlight(old_conn_state, 0);
+	intel_panel_set_pwm_level(old_conn_state, val);
 
 	tmp = intel_de_read(dev_priv,
 			    BXT_BLC_PWM_CTL(panel->backlight.controller));
@@ -835,7 +843,7 @@ static void cnp_disable_backlight(const struct drm_connector_state *old_conn_sta
 		       tmp & ~BXT_BLC_PWM_ENABLE);
 }
 
-static void ext_pwm_disable_backlight(const struct drm_connector_state *old_conn_state)
+static void ext_pwm_disable_backlight(const struct drm_connector_state *old_conn_state, u32 level)
 {
 	struct intel_connector *connector = to_intel_connector(old_conn_state->connector);
 	struct intel_panel *panel = &connector->panel;
@@ -876,7 +884,7 @@ void intel_panel_disable_backlight(const struct drm_connector_state *old_conn_st
 }
 
 static void lpt_enable_backlight(const struct intel_crtc_state *crtc_state,
-				 const struct drm_connector_state *conn_state)
+				 const struct drm_connector_state *conn_state, u32 level)
 {
 	struct intel_connector *connector = to_intel_connector(conn_state->connector);
 	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
@@ -906,7 +914,7 @@ static void lpt_enable_backlight(const struct intel_crtc_state *crtc_state,
 		intel_de_write(dev_priv, SOUTH_CHICKEN1, schicken);
 	}
 
-	pch_ctl2 = panel->backlight.max << 16;
+	pch_ctl2 = panel->backlight.pwm_max << 16;
 	intel_de_write(dev_priv, BLC_PWM_PCH_CTL2, pch_ctl2);
 
 	pch_ctl1 = 0;
@@ -923,11 +931,11 @@ static void lpt_enable_backlight(const struct intel_crtc_state *crtc_state,
 		       pch_ctl1 | BLM_PCH_PWM_ENABLE);
 
 	/* This won't stick until the above enable. */
-	intel_panel_actually_set_backlight(conn_state, panel->backlight.level);
+	intel_panel_set_pwm_level(conn_state, level);
 }
 
 static void pch_enable_backlight(const struct intel_crtc_state *crtc_state,
-				 const struct drm_connector_state *conn_state)
+				 const struct drm_connector_state *conn_state, u32 level)
 {
 	struct intel_connector *connector = to_intel_connector(conn_state->connector);
 	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
@@ -958,9 +966,9 @@ static void pch_enable_backlight(const struct intel_crtc_state *crtc_state,
 	intel_de_write(dev_priv, BLC_PWM_CPU_CTL2, cpu_ctl2 | BLM_PWM_ENABLE);
 
 	/* This won't stick until the above enable. */
-	intel_panel_actually_set_backlight(conn_state, panel->backlight.level);
+	intel_panel_set_pwm_level(conn_state, level);
 
-	pch_ctl2 = panel->backlight.max << 16;
+	pch_ctl2 = panel->backlight.pwm_max << 16;
 	intel_de_write(dev_priv, BLC_PWM_PCH_CTL2, pch_ctl2);
 
 	pch_ctl1 = 0;
@@ -974,7 +982,7 @@ static void pch_enable_backlight(const struct intel_crtc_state *crtc_state,
 }
 
 static void i9xx_enable_backlight(const struct intel_crtc_state *crtc_state,
-				  const struct drm_connector_state *conn_state)
+				  const struct drm_connector_state *conn_state, u32 level)
 {
 	struct intel_connector *connector = to_intel_connector(conn_state->connector);
 	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
@@ -987,7 +995,7 @@ static void i9xx_enable_backlight(const struct intel_crtc_state *crtc_state,
 		intel_de_write(dev_priv, BLC_PWM_CTL, 0);
 	}
 
-	freq = panel->backlight.max;
+	freq = panel->backlight.pwm_max;
 	if (panel->backlight.combination_mode)
 		freq /= 0xff;
 
@@ -1001,7 +1009,7 @@ static void i9xx_enable_backlight(const struct intel_crtc_state *crtc_state,
 	intel_de_posting_read(dev_priv, BLC_PWM_CTL);
 
 	/* XXX: combine this into above write? */
-	intel_panel_actually_set_backlight(conn_state, panel->backlight.level);
+	intel_panel_set_pwm_level(conn_state, level);
 
 	/*
 	 * Needed to enable backlight on some 855gm models. BLC_HIST_CTL is
@@ -1013,7 +1021,7 @@ static void i9xx_enable_backlight(const struct intel_crtc_state *crtc_state,
 }
 
 static void i965_enable_backlight(const struct intel_crtc_state *crtc_state,
-				  const struct drm_connector_state *conn_state)
+				  const struct drm_connector_state *conn_state, u32 level)
 {
 	struct intel_connector *connector = to_intel_connector(conn_state->connector);
 	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
@@ -1028,7 +1036,7 @@ static void i965_enable_backlight(const struct intel_crtc_state *crtc_state,
 		intel_de_write(dev_priv, BLC_PWM_CTL2, ctl2);
 	}
 
-	freq = panel->backlight.max;
+	freq = panel->backlight.pwm_max;
 	if (panel->backlight.combination_mode)
 		freq /= 0xff;
 
@@ -1044,11 +1052,11 @@ static void i965_enable_backlight(const struct intel_crtc_state *crtc_state,
 	intel_de_posting_read(dev_priv, BLC_PWM_CTL2);
 	intel_de_write(dev_priv, BLC_PWM_CTL2, ctl2 | BLM_PWM_ENABLE);
 
-	intel_panel_actually_set_backlight(conn_state, panel->backlight.level);
+	intel_panel_set_pwm_level(conn_state, level);
 }
 
 static void vlv_enable_backlight(const struct intel_crtc_state *crtc_state,
-				 const struct drm_connector_state *conn_state)
+				 const struct drm_connector_state *conn_state, u32 level)
 {
 	struct intel_connector *connector = to_intel_connector(conn_state->connector);
 	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
@@ -1063,11 +1071,11 @@ static void vlv_enable_backlight(const struct intel_crtc_state *crtc_state,
 		intel_de_write(dev_priv, VLV_BLC_PWM_CTL2(pipe), ctl2);
 	}
 
-	ctl = panel->backlight.max << 16;
+	ctl = panel->backlight.pwm_max << 16;
 	intel_de_write(dev_priv, VLV_BLC_PWM_CTL(pipe), ctl);
 
 	/* XXX: combine this into above write? */
-	intel_panel_actually_set_backlight(conn_state, panel->backlight.level);
+	intel_panel_set_pwm_level(conn_state, level);
 
 	ctl2 = 0;
 	if (panel->backlight.active_low_pwm)
@@ -1079,7 +1087,7 @@ static void vlv_enable_backlight(const struct intel_crtc_state *crtc_state,
 }
 
 static void bxt_enable_backlight(const struct intel_crtc_state *crtc_state,
-				 const struct drm_connector_state *conn_state)
+				 const struct drm_connector_state *conn_state, u32 level)
 {
 	struct intel_connector *connector = to_intel_connector(conn_state->connector);
 	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
@@ -1116,9 +1124,9 @@ static void bxt_enable_backlight(const struct intel_crtc_state *crtc_state,
 
 	intel_de_write(dev_priv,
 		       BXT_BLC_PWM_FREQ(panel->backlight.controller),
-		       panel->backlight.max);
+		       panel->backlight.pwm_max);
 
-	intel_panel_actually_set_backlight(conn_state, panel->backlight.level);
+	intel_panel_set_pwm_level(conn_state, level);
 
 	pwm_ctl = 0;
 	if (panel->backlight.active_low_pwm)
@@ -1133,7 +1141,7 @@ static void bxt_enable_backlight(const struct intel_crtc_state *crtc_state,
 }
 
 static void cnp_enable_backlight(const struct intel_crtc_state *crtc_state,
-				 const struct drm_connector_state *conn_state)
+				 const struct drm_connector_state *conn_state, u32 level)
 {
 	struct intel_connector *connector = to_intel_connector(conn_state->connector);
 	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
@@ -1152,9 +1160,9 @@ static void cnp_enable_backlight(const struct intel_crtc_state *crtc_state,
 
 	intel_de_write(dev_priv,
 		       BXT_BLC_PWM_FREQ(panel->backlight.controller),
-		       panel->backlight.max);
+		       panel->backlight.pwm_max);
 
-	intel_panel_actually_set_backlight(conn_state, panel->backlight.level);
+	intel_panel_set_pwm_level(conn_state, level);
 
 	pwm_ctl = 0;
 	if (panel->backlight.active_low_pwm)
@@ -1169,13 +1177,11 @@ static void cnp_enable_backlight(const struct intel_crtc_state *crtc_state,
 }
 
 static void ext_pwm_enable_backlight(const struct intel_crtc_state *crtc_state,
-				     const struct drm_connector_state *conn_state)
+				     const struct drm_connector_state *conn_state, u32 level)
 {
 	struct intel_connector *connector = to_intel_connector(conn_state->connector);
 	struct intel_panel *panel = &connector->panel;
-	int level = panel->backlight.level;
 
-	level = intel_panel_compute_brightness(connector, level);
 	pwm_set_relative_duty_cycle(&panel->backlight.pwm_state, level, 100);
 	panel->backlight.pwm_state.enabled = true;
 	pwm_apply_state(panel->backlight.pwm, &panel->backlight.pwm_state);
@@ -1233,10 +1239,8 @@ static u32 intel_panel_get_backlight(struct intel_connector *connector)
 
 	mutex_lock(&dev_priv->backlight_lock);
 
-	if (panel->backlight.enabled) {
+	if (panel->backlight.enabled)
 		val = panel->backlight.get(connector);
-		val = intel_panel_compute_brightness(connector, val);
-	}
 
 	mutex_unlock(&dev_priv->backlight_lock);
 
@@ -1567,13 +1571,13 @@ static u32 get_backlight_max_vbt(struct intel_connector *connector)
 	u16 pwm_freq_hz = get_vbt_pwm_freq(dev_priv);
 	u32 pwm;
 
-	if (!panel->backlight.hz_to_pwm) {
+	if (!panel->backlight.pwm_funcs.hz_to_pwm) {
 		drm_dbg_kms(&dev_priv->drm,
 			    "backlight frequency conversion not supported\n");
 		return 0;
 	}
 
-	pwm = panel->backlight.hz_to_pwm(connector, pwm_freq_hz);
+	pwm = panel->backlight.pwm_funcs.hz_to_pwm(connector, pwm_freq_hz);
 	if (!pwm) {
 		drm_dbg_kms(&dev_priv->drm,
 			    "backlight frequency conversion failed\n");
@@ -1592,7 +1596,7 @@ static u32 get_backlight_min_vbt(struct intel_connector *connector)
 	struct intel_panel *panel = &connector->panel;
 	int min;
 
-	drm_WARN_ON(&dev_priv->drm, panel->backlight.max == 0);
+	drm_WARN_ON(&dev_priv->drm, panel->backlight.pwm_max == 0);
 
 	/*
 	 * XXX: If the vbt value is 255, it makes min equal to max, which leads
@@ -1609,7 +1613,7 @@ static u32 get_backlight_min_vbt(struct intel_connector *connector)
 	}
 
 	/* vbt value is a coefficient in range [0..255] */
-	return scale(min, 0, 255, 0, panel->backlight.max);
+	return scale(min, 0, 255, 0, panel->backlight.pwm_max);
 }
 
 static int lpt_setup_backlight(struct intel_connector *connector, enum pipe unused)
@@ -1629,37 +1633,32 @@ static int lpt_setup_backlight(struct intel_connector *connector, enum pipe unus
 	panel->backlight.active_low_pwm = pch_ctl1 & BLM_PCH_POLARITY;
 
 	pch_ctl2 = intel_de_read(dev_priv, BLC_PWM_PCH_CTL2);
-	panel->backlight.max = pch_ctl2 >> 16;
+	panel->backlight.pwm_max = pch_ctl2 >> 16;
 
 	cpu_ctl2 = intel_de_read(dev_priv, BLC_PWM_CPU_CTL2);
 
-	if (!panel->backlight.max)
-		panel->backlight.max = get_backlight_max_vbt(connector);
+	if (!panel->backlight.pwm_max)
+		panel->backlight.pwm_max = get_backlight_max_vbt(connector);
 
-	if (!panel->backlight.max)
+	if (!panel->backlight.pwm_max)
 		return -ENODEV;
 
-	panel->backlight.min = get_backlight_min_vbt(connector);
+	panel->backlight.pwm_min = get_backlight_min_vbt(connector);
 
-	panel->backlight.enabled = pch_ctl1 & BLM_PCH_PWM_ENABLE;
+	panel->backlight.pwm_enabled = pch_ctl1 & BLM_PCH_PWM_ENABLE;
 
-	cpu_mode = panel->backlight.enabled && HAS_PCH_LPT(dev_priv) &&
+	cpu_mode = panel->backlight.pwm_enabled && HAS_PCH_LPT(dev_priv) &&
 		   !(pch_ctl1 & BLM_PCH_OVERRIDE_ENABLE) &&
 		   (cpu_ctl2 & BLM_PWM_ENABLE);
-	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);
 
@@ -1674,29 +1673,24 @@ static int pch_setup_backlight(struct intel_connector *connector, enum pipe unus
 {
 	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
 	struct intel_panel *panel = &connector->panel;
-	u32 cpu_ctl2, pch_ctl1, pch_ctl2, val;
+	u32 cpu_ctl2, pch_ctl1, pch_ctl2;
 
 	pch_ctl1 = intel_de_read(dev_priv, BLC_PWM_PCH_CTL1);
 	panel->backlight.active_low_pwm = pch_ctl1 & BLM_PCH_POLARITY;
 
 	pch_ctl2 = intel_de_read(dev_priv, BLC_PWM_PCH_CTL2);
-	panel->backlight.max = pch_ctl2 >> 16;
+	panel->backlight.pwm_max = pch_ctl2 >> 16;
 
-	if (!panel->backlight.max)
-		panel->backlight.max = get_backlight_max_vbt(connector);
+	if (!panel->backlight.pwm_max)
+		panel->backlight.pwm_max = get_backlight_max_vbt(connector);
 
-	if (!panel->backlight.max)
+	if (!panel->backlight.pwm_max)
 		return -ENODEV;
 
-	panel->backlight.min = get_backlight_min_vbt(connector);
-
-	val = pch_get_backlight(connector);
-	val = intel_panel_compute_brightness(connector, val);
-	panel->backlight.level = clamp(val, panel->backlight.min,
-				       panel->backlight.max);
+	panel->backlight.pwm_min = get_backlight_min_vbt(connector);
 
 	cpu_ctl2 = intel_de_read(dev_priv, BLC_PWM_CPU_CTL2);
-	panel->backlight.enabled = (cpu_ctl2 & BLM_PWM_ENABLE) &&
+	panel->backlight.pwm_enabled = (cpu_ctl2 & BLM_PWM_ENABLE) &&
 		(pch_ctl1 & BLM_PCH_PWM_ENABLE);
 
 	return 0;
@@ -1716,27 +1710,26 @@ static int i9xx_setup_backlight(struct intel_connector *connector, enum pipe unu
 	if (IS_PINEVIEW(dev_priv))
 		panel->backlight.active_low_pwm = ctl & BLM_POLARITY_PNV;
 
-	panel->backlight.max = ctl >> 17;
+	panel->backlight.pwm_max = ctl >> 17;
 
-	if (!panel->backlight.max) {
-		panel->backlight.max = get_backlight_max_vbt(connector);
-		panel->backlight.max >>= 1;
+	if (!panel->backlight.pwm_max) {
+		panel->backlight.pwm_max = get_backlight_max_vbt(connector);
+		panel->backlight.pwm_max >>= 1;
 	}
 
-	if (!panel->backlight.max)
+	if (!panel->backlight.pwm_max)
 		return -ENODEV;
 
 	if (panel->backlight.combination_mode)
-		panel->backlight.max *= 0xff;
+		panel->backlight.pwm_max *= 0xff;
 
-	panel->backlight.min = get_backlight_min_vbt(connector);
+	panel->backlight.pwm_min = get_backlight_min_vbt(connector);
 
 	val = i9xx_get_backlight(connector);
-	val = intel_panel_compute_brightness(connector, val);
-	panel->backlight.level = clamp(val, panel->backlight.min,
-				       panel->backlight.max);
+	val = intel_panel_sanitize_pwm_level(connector, val);
+	val = clamp(val, panel->backlight.pwm_min, panel->backlight.pwm_max);
 
-	panel->backlight.enabled = val != 0;
+	panel->backlight.pwm_enabled = val != 0;
 
 	return 0;
 }
@@ -1745,32 +1738,27 @@ static int i965_setup_backlight(struct intel_connector *connector, enum pipe unu
 {
 	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
 	struct intel_panel *panel = &connector->panel;
-	u32 ctl, ctl2, val;
+	u32 ctl, ctl2;
 
 	ctl2 = intel_de_read(dev_priv, BLC_PWM_CTL2);
 	panel->backlight.combination_mode = ctl2 & BLM_COMBINATION_MODE;
 	panel->backlight.active_low_pwm = ctl2 & BLM_POLARITY_I965;
 
 	ctl = intel_de_read(dev_priv, BLC_PWM_CTL);
-	panel->backlight.max = ctl >> 16;
+	panel->backlight.pwm_max = ctl >> 16;
 
-	if (!panel->backlight.max)
-		panel->backlight.max = get_backlight_max_vbt(connector);
+	if (!panel->backlight.pwm_max)
+		panel->backlight.pwm_max = get_backlight_max_vbt(connector);
 
-	if (!panel->backlight.max)
+	if (!panel->backlight.pwm_max)
 		return -ENODEV;
 
 	if (panel->backlight.combination_mode)
-		panel->backlight.max *= 0xff;
+		panel->backlight.pwm_max *= 0xff;
 
-	panel->backlight.min = get_backlight_min_vbt(connector);
+	panel->backlight.pwm_min = get_backlight_min_vbt(connector);
 
-	val = i9xx_get_backlight(connector);
-	val = intel_panel_compute_brightness(connector, val);
-	panel->backlight.level = clamp(val, panel->backlight.min,
-				       panel->backlight.max);
-
-	panel->backlight.enabled = ctl2 & BLM_PWM_ENABLE;
+	panel->backlight.pwm_enabled = ctl2 & BLM_PWM_ENABLE;
 
 	return 0;
 }
@@ -1779,7 +1767,7 @@ static int vlv_setup_backlight(struct intel_connector *connector, enum pipe pipe
 {
 	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
 	struct intel_panel *panel = &connector->panel;
-	u32 ctl, ctl2, val;
+	u32 ctl, ctl2;
 
 	if (drm_WARN_ON(&dev_priv->drm, pipe != PIPE_A && pipe != PIPE_B))
 		return -ENODEV;
@@ -1788,22 +1776,17 @@ static int vlv_setup_backlight(struct intel_connector *connector, enum pipe pipe
 	panel->backlight.active_low_pwm = ctl2 & BLM_POLARITY_I965;
 
 	ctl = intel_de_read(dev_priv, VLV_BLC_PWM_CTL(pipe));
-	panel->backlight.max = ctl >> 16;
+	panel->backlight.pwm_max = ctl >> 16;
 
-	if (!panel->backlight.max)
-		panel->backlight.max = get_backlight_max_vbt(connector);
+	if (!panel->backlight.pwm_max)
+		panel->backlight.pwm_max = get_backlight_max_vbt(connector);
 
-	if (!panel->backlight.max)
+	if (!panel->backlight.pwm_max)
 		return -ENODEV;
 
-	panel->backlight.min = get_backlight_min_vbt(connector);
+	panel->backlight.pwm_min = get_backlight_min_vbt(connector);
 
-	val = _vlv_get_backlight(dev_priv, pipe);
-	val = intel_panel_compute_brightness(connector, val);
-	panel->backlight.level = clamp(val, panel->backlight.min,
-				       panel->backlight.max);
-
-	panel->backlight.enabled = ctl2 & BLM_PWM_ENABLE;
+	panel->backlight.pwm_enabled = ctl2 & BLM_PWM_ENABLE;
 
 	return 0;
 }
@@ -1828,24 +1811,18 @@ bxt_setup_backlight(struct intel_connector *connector, enum pipe unused)
 	}
 
 	panel->backlight.active_low_pwm = pwm_ctl & BXT_BLC_PWM_POLARITY;
-	panel->backlight.max =
-		intel_de_read(dev_priv,
-			      BXT_BLC_PWM_FREQ(panel->backlight.controller));
+	panel->backlight.pwm_max = intel_de_read(dev_priv,
+						 BXT_BLC_PWM_FREQ(panel->backlight.controller));
 
-	if (!panel->backlight.max)
-		panel->backlight.max = get_backlight_max_vbt(connector);
+	if (!panel->backlight.pwm_max)
+		panel->backlight.pwm_max = get_backlight_max_vbt(connector);
 
-	if (!panel->backlight.max)
+	if (!panel->backlight.pwm_max)
 		return -ENODEV;
 
-	panel->backlight.min = get_backlight_min_vbt(connector);
-
-	val = bxt_get_backlight(connector);
-	val = intel_panel_compute_brightness(connector, val);
-	panel->backlight.level = clamp(val, panel->backlight.min,
-				       panel->backlight.max);
+	panel->backlight.pwm_min = get_backlight_min_vbt(connector);
 
-	panel->backlight.enabled = pwm_ctl & BXT_BLC_PWM_ENABLE;
+	panel->backlight.pwm_enabled = pwm_ctl & BXT_BLC_PWM_ENABLE;
 
 	return 0;
 }
@@ -1855,7 +1832,7 @@ cnp_setup_backlight(struct intel_connector *connector, enum pipe unused)
 {
 	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
 	struct intel_panel *panel = &connector->panel;
-	u32 pwm_ctl, val;
+	u32 pwm_ctl;
 
 	/*
 	 * CNP has the BXT implementation of backlight, but with only one
@@ -1868,24 +1845,18 @@ cnp_setup_backlight(struct intel_connector *connector, enum pipe unused)
 				BXT_BLC_PWM_CTL(panel->backlight.controller));
 
 	panel->backlight.active_low_pwm = pwm_ctl & BXT_BLC_PWM_POLARITY;
-	panel->backlight.max =
-		intel_de_read(dev_priv,
-			      BXT_BLC_PWM_FREQ(panel->backlight.controller));
+	panel->backlight.pwm_max = intel_de_read(dev_priv,
+						 BXT_BLC_PWM_FREQ(panel->backlight.controller));
 
-	if (!panel->backlight.max)
-		panel->backlight.max = get_backlight_max_vbt(connector);
+	if (!panel->backlight.pwm_max)
+		panel->backlight.pwm_max = get_backlight_max_vbt(connector);
 
-	if (!panel->backlight.max)
+	if (!panel->backlight.pwm_max)
 		return -ENODEV;
 
-	panel->backlight.min = get_backlight_min_vbt(connector);
-
-	val = bxt_get_backlight(connector);
-	val = intel_panel_compute_brightness(connector, val);
-	panel->backlight.level = clamp(val, panel->backlight.min,
-				       panel->backlight.max);
+	panel->backlight.pwm_min = get_backlight_min_vbt(connector);
 
-	panel->backlight.enabled = pwm_ctl & BXT_BLC_PWM_ENABLE;
+	panel->backlight.pwm_enabled = pwm_ctl & BXT_BLC_PWM_ENABLE;
 
 	return 0;
 }
@@ -1915,8 +1886,8 @@ static int ext_pwm_setup_backlight(struct intel_connector *connector,
 		return -ENODEV;
 	}
 
-	panel->backlight.max = 100; /* 100% */
-	panel->backlight.min = get_backlight_min_vbt(connector);
+	panel->backlight.pwm_max = 100; /* 100% */
+	panel->backlight.pwm_min = get_backlight_min_vbt(connector);
 
 	if (pwm_is_enabled(panel->backlight.pwm)) {
 		/* PWM is already enabled, use existing settings */
@@ -1924,10 +1895,8 @@ static int ext_pwm_setup_backlight(struct intel_connector *connector,
 
 		level = pwm_get_relative_duty_cycle(&panel->backlight.pwm_state,
 						    100);
-		level = intel_panel_compute_brightness(connector, level);
-		panel->backlight.level = clamp(level, panel->backlight.min,
-					       panel->backlight.max);
-		panel->backlight.enabled = true;
+		level = intel_panel_sanitize_pwm_level(connector, level);
+		panel->backlight.pwm_enabled = true;
 
 		drm_dbg_kms(&dev_priv->drm, "PWM already enabled at freq %ld, VBT freq %d, level %d\n",
 			    NSEC_PER_SEC / (unsigned long)panel->backlight.pwm_state.period,
@@ -1943,6 +1912,60 @@ static int ext_pwm_setup_backlight(struct intel_connector *connector,
 	return 0;
 }
 
+static void intel_pwm_set_backlight(const struct drm_connector_state *conn_state, u32 level)
+{
+	struct intel_connector *connector = to_intel_connector(conn_state->connector);
+	struct intel_panel *panel = &connector->panel;
+
+	panel->backlight.pwm_funcs.set(conn_state,
+				       intel_panel_sanitize_pwm_level(connector, level));
+}
+
+static u32 intel_pwm_get_backlight(struct intel_connector *connector)
+{
+	struct intel_panel *panel = &connector->panel;
+
+	return intel_panel_sanitize_pwm_level(connector, panel->backlight.pwm_funcs.get(connector));
+}
+
+static void intel_pwm_enable_backlight(const struct intel_crtc_state *crtc_state,
+				       const struct drm_connector_state *conn_state)
+{
+	struct intel_connector *connector = to_intel_connector(conn_state->connector);
+	struct intel_panel *panel = &connector->panel;
+	u32 level = intel_panel_sanitize_pwm_level(connector, panel->backlight.level);
+
+	panel->backlight.pwm_funcs.enable(crtc_state, conn_state, level);
+}
+
+static void intel_pwm_disable_backlight(const struct drm_connector_state *conn_state)
+{
+	struct intel_connector *connector = to_intel_connector(conn_state->connector);
+	struct intel_panel *panel = &connector->panel;
+
+	panel->backlight.pwm_funcs.disable(conn_state,
+					   intel_panel_sanitize_pwm_level(connector, 0));
+}
+
+/* The only hook PWM-only backlight setups need to override, the rest of the hooks are filled in
+ * from pwm_funcs
+ */
+static int intel_pwm_setup_backlight(struct intel_connector *connector, enum pipe pipe)
+{
+	struct intel_panel *panel = &connector->panel;
+	int ret = panel->backlight.pwm_funcs.setup(connector, pipe);
+
+	if (ret < 0)
+		return ret;
+
+	panel->backlight.min = panel->backlight.pwm_min;
+	panel->backlight.max = panel->backlight.pwm_max;
+	panel->backlight.level = panel->backlight.get(connector);
+	panel->backlight.enabled = panel->backlight.pwm_enabled;
+
+	return 0;
+}
+
 void intel_panel_update_backlight(struct intel_atomic_state *state,
 				  struct intel_encoder *encoder,
 				  const struct intel_crtc_state *crtc_state,
@@ -2024,75 +2047,82 @@ intel_panel_init_backlight_funcs(struct intel_panel *panel)
 		container_of(panel, struct intel_connector, panel);
 	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
 
-	if (connector->base.connector_type == DRM_MODE_CONNECTOR_eDP &&
-	    intel_dp_aux_init_backlight_funcs(connector) == 0)
-		return;
-
 	if (connector->base.connector_type == DRM_MODE_CONNECTOR_DSI &&
 	    intel_dsi_dcs_init_backlight_funcs(connector) == 0)
 		return;
 
 	if (IS_GEN9_LP(dev_priv)) {
-		panel->backlight.setup = bxt_setup_backlight;
-		panel->backlight.enable = bxt_enable_backlight;
-		panel->backlight.disable = bxt_disable_backlight;
-		panel->backlight.set = bxt_set_backlight;
-		panel->backlight.get = bxt_get_backlight;
-		panel->backlight.hz_to_pwm = bxt_hz_to_pwm;
+		panel->backlight.pwm_funcs.setup = bxt_setup_backlight;
+		panel->backlight.pwm_funcs.enable = bxt_enable_backlight;
+		panel->backlight.pwm_funcs.disable = bxt_disable_backlight;
+		panel->backlight.pwm_funcs.set = bxt_set_backlight;
+		panel->backlight.pwm_funcs.get = bxt_get_backlight;
+		panel->backlight.pwm_funcs.hz_to_pwm = bxt_hz_to_pwm;
 	} else if (INTEL_PCH_TYPE(dev_priv) >= PCH_CNP) {
-		panel->backlight.setup = cnp_setup_backlight;
-		panel->backlight.enable = cnp_enable_backlight;
-		panel->backlight.disable = cnp_disable_backlight;
-		panel->backlight.set = bxt_set_backlight;
-		panel->backlight.get = bxt_get_backlight;
-		panel->backlight.hz_to_pwm = cnp_hz_to_pwm;
+		panel->backlight.pwm_funcs.setup = cnp_setup_backlight;
+		panel->backlight.pwm_funcs.enable = cnp_enable_backlight;
+		panel->backlight.pwm_funcs.disable = cnp_disable_backlight;
+		panel->backlight.pwm_funcs.set = bxt_set_backlight;
+		panel->backlight.pwm_funcs.get = bxt_get_backlight;
+		panel->backlight.pwm_funcs.hz_to_pwm = cnp_hz_to_pwm;
 	} else if (INTEL_PCH_TYPE(dev_priv) >= PCH_LPT) {
-		panel->backlight.setup = lpt_setup_backlight;
-		panel->backlight.enable = lpt_enable_backlight;
-		panel->backlight.disable = lpt_disable_backlight;
-		panel->backlight.set = lpt_set_backlight;
-		panel->backlight.get = lpt_get_backlight;
+		panel->backlight.pwm_funcs.setup = lpt_setup_backlight;
+		panel->backlight.pwm_funcs.enable = lpt_enable_backlight;
+		panel->backlight.pwm_funcs.disable = lpt_disable_backlight;
+		panel->backlight.pwm_funcs.set = lpt_set_backlight;
+		panel->backlight.pwm_funcs.get = lpt_get_backlight;
 		if (HAS_PCH_LPT(dev_priv))
-			panel->backlight.hz_to_pwm = lpt_hz_to_pwm;
+			panel->backlight.pwm_funcs.hz_to_pwm = lpt_hz_to_pwm;
 		else
-			panel->backlight.hz_to_pwm = spt_hz_to_pwm;
+			panel->backlight.pwm_funcs.hz_to_pwm = spt_hz_to_pwm;
 	} else if (HAS_PCH_SPLIT(dev_priv)) {
-		panel->backlight.setup = pch_setup_backlight;
-		panel->backlight.enable = pch_enable_backlight;
-		panel->backlight.disable = pch_disable_backlight;
-		panel->backlight.set = pch_set_backlight;
-		panel->backlight.get = pch_get_backlight;
-		panel->backlight.hz_to_pwm = pch_hz_to_pwm;
+		panel->backlight.pwm_funcs.setup = pch_setup_backlight;
+		panel->backlight.pwm_funcs.enable = pch_enable_backlight;
+		panel->backlight.pwm_funcs.disable = pch_disable_backlight;
+		panel->backlight.pwm_funcs.set = pch_set_backlight;
+		panel->backlight.pwm_funcs.get = pch_get_backlight;
+		panel->backlight.pwm_funcs.hz_to_pwm = pch_hz_to_pwm;
 	} else if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
 		if (connector->base.connector_type == DRM_MODE_CONNECTOR_DSI) {
-			panel->backlight.setup = ext_pwm_setup_backlight;
-			panel->backlight.enable = ext_pwm_enable_backlight;
-			panel->backlight.disable = ext_pwm_disable_backlight;
-			panel->backlight.set = ext_pwm_set_backlight;
-			panel->backlight.get = ext_pwm_get_backlight;
+			panel->backlight.pwm_funcs.setup = ext_pwm_setup_backlight;
+			panel->backlight.pwm_funcs.enable = ext_pwm_enable_backlight;
+			panel->backlight.pwm_funcs.disable = ext_pwm_disable_backlight;
+			panel->backlight.pwm_funcs.set = ext_pwm_set_backlight;
+			panel->backlight.pwm_funcs.get = ext_pwm_get_backlight;
 		} else {
-			panel->backlight.setup = vlv_setup_backlight;
-			panel->backlight.enable = vlv_enable_backlight;
-			panel->backlight.disable = vlv_disable_backlight;
-			panel->backlight.set = vlv_set_backlight;
-			panel->backlight.get = vlv_get_backlight;
-			panel->backlight.hz_to_pwm = vlv_hz_to_pwm;
+			panel->backlight.pwm_funcs.setup = vlv_setup_backlight;
+			panel->backlight.pwm_funcs.enable = vlv_enable_backlight;
+			panel->backlight.pwm_funcs.disable = vlv_disable_backlight;
+			panel->backlight.pwm_funcs.set = vlv_set_backlight;
+			panel->backlight.pwm_funcs.get = vlv_get_backlight;
+			panel->backlight.pwm_funcs.hz_to_pwm = vlv_hz_to_pwm;
 		}
 	} else if (IS_GEN(dev_priv, 4)) {
-		panel->backlight.setup = i965_setup_backlight;
-		panel->backlight.enable = i965_enable_backlight;
-		panel->backlight.disable = i965_disable_backlight;
-		panel->backlight.set = i9xx_set_backlight;
-		panel->backlight.get = i9xx_get_backlight;
-		panel->backlight.hz_to_pwm = i965_hz_to_pwm;
+		panel->backlight.pwm_funcs.setup = i965_setup_backlight;
+		panel->backlight.pwm_funcs.enable = i965_enable_backlight;
+		panel->backlight.pwm_funcs.disable = i965_disable_backlight;
+		panel->backlight.pwm_funcs.set = i9xx_set_backlight;
+		panel->backlight.pwm_funcs.get = i9xx_get_backlight;
+		panel->backlight.pwm_funcs.hz_to_pwm = i965_hz_to_pwm;
 	} else {
-		panel->backlight.setup = i9xx_setup_backlight;
-		panel->backlight.enable = i9xx_enable_backlight;
-		panel->backlight.disable = i9xx_disable_backlight;
-		panel->backlight.set = i9xx_set_backlight;
-		panel->backlight.get = i9xx_get_backlight;
-		panel->backlight.hz_to_pwm = i9xx_hz_to_pwm;
+		panel->backlight.pwm_funcs.setup = i9xx_setup_backlight;
+		panel->backlight.pwm_funcs.enable = i9xx_enable_backlight;
+		panel->backlight.pwm_funcs.disable = i9xx_disable_backlight;
+		panel->backlight.pwm_funcs.set = i9xx_set_backlight;
+		panel->backlight.pwm_funcs.get = i9xx_get_backlight;
+		panel->backlight.pwm_funcs.hz_to_pwm = i9xx_hz_to_pwm;
 	}
+
+	if (connector->base.connector_type == DRM_MODE_CONNECTOR_eDP &&
+	    intel_dp_aux_init_backlight_funcs(connector) == 0)
+		return;
+
+	/* We're using a standard PWM backlight interface */
+	panel->backlight.setup = intel_pwm_setup_backlight;
+	panel->backlight.enable = intel_pwm_enable_backlight;
+	panel->backlight.disable = intel_pwm_disable_backlight;
+	panel->backlight.set = intel_pwm_set_backlight;
+	panel->backlight.get = intel_pwm_get_backlight;
 }
 
 enum drm_connector_status
-- 
2.26.2

Re: [Intel-gfx] [RFC v2 3/8] drm/i915: Keep track of pwm-related backlight hooks separately

[Rodrigo Vivi]

On Wed, Sep 16, 2020 at 01:18:50PM -0400, Lyude Paul 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.
> 
> Signed-off-by: Lyude Paul <lyude@redhat.com>
> Cc: thaytan@noraisin.net
> Cc: Vasily Khoruzhick <anarsoul@gmail.com>
> ---
>  .../drm/i915/display/intel_display_types.h    |  14 +-
>  drivers/gpu/drm/i915/display/intel_panel.c    | 436 ++++++++++--------
>  2 files changed, 246 insertions(+), 204 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
> index b2d0edacc58c9..52a6543df842a 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> @@ -221,6 +221,9 @@ struct intel_panel {
>  		bool alternate_pwm_increment;	/* lpt+ */
>  
>  		/* PWM chip */
> +		u32 pwm_min;
> +		u32 pwm_max;
> +		bool pwm_enabled;
>  		bool util_pin_active_low;	/* bxt+ */
>  		u8 controller;		/* bxt+ only */
>  		struct pwm_device *pwm;
> @@ -229,6 +232,16 @@ struct intel_panel {
>  		/* DPCD backlight */
>  		u8 pwmgen_bit_count;
>  
> +		struct {
> +			int (*setup)(struct intel_connector *connector, enum pipe pipe);
> +			u32 (*get)(struct intel_connector *connector);
> +			void (*set)(const struct drm_connector_state *conn_state, u32 level);
> +			void (*disable)(const struct drm_connector_state *conn_state, u32 level);
> +			void (*enable)(const struct intel_crtc_state *crtc_state,
> +				       const struct drm_connector_state *conn_state, u32 level);
> +			u32 (*hz_to_pwm)(struct intel_connector *connector, u32 hz);
> +		} pwm_funcs;
> +
>  		struct backlight_device *device;
>  
>  		/* Connector and platform specific backlight functions */
> @@ -238,7 +251,6 @@ struct intel_panel {
>  		void (*disable)(const struct drm_connector_state *conn_state);
>  		void (*enable)(const struct intel_crtc_state *crtc_state,
>  			       const struct drm_connector_state *conn_state);
> -		u32 (*hz_to_pwm)(struct intel_connector *connector, u32 hz);

I see my poor brain getting confused with these 2 levels with very similar function sets
with same name.
But I currently have no suggestion for better organization or names...

>  		void (*power)(struct intel_connector *, bool enable);
>  	} backlight;
>  };
> diff --git a/drivers/gpu/drm/i915/display/intel_panel.c b/drivers/gpu/drm/i915/display/intel_panel.c
> index c0e36244bb07d..6d3e9d51d069c 100644
> --- a/drivers/gpu/drm/i915/display/intel_panel.c
> +++ b/drivers/gpu/drm/i915/display/intel_panel.c
> @@ -511,25 +511,34 @@ static u32 scale_hw_to_user(struct intel_connector *connector,
>  		     0, user_max);
>  }
>  
> -static u32 intel_panel_compute_brightness(struct intel_connector *connector,
> -					  u32 val)
> +static u32 intel_panel_sanitize_pwm_level(struct intel_connector *connector, u32 val)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
>  	struct intel_panel *panel = &connector->panel;
>  
> -	drm_WARN_ON(&dev_priv->drm, panel->backlight.max == 0);
> +	drm_WARN_ON(&dev_priv->drm, panel->backlight.pwm_max == 0);
>  
>  	if (dev_priv->params.invert_brightness < 0)
>  		return val;
>  
>  	if (dev_priv->params.invert_brightness > 0 ||
>  	    dev_priv->quirks & QUIRK_INVERT_BRIGHTNESS) {
> -		return panel->backlight.max - val + panel->backlight.min;
> +		return panel->backlight.pwm_max - val + panel->backlight.pwm_min;
>  	}
>  
>  	return val;
>  }
>  
> +static void intel_panel_set_pwm_level(const struct drm_connector_state *conn_state, u32 val)
> +{
> +	struct intel_connector *connector = to_intel_connector(conn_state->connector);
> +	struct drm_i915_private *i915 = to_i915(connector->base.dev);
> +	struct intel_panel *panel = &connector->panel;
> +
> +	drm_dbg_kms(&i915->drm, "set backlight PWM = %d\n", val);
> +	panel->backlight.pwm_funcs.set(conn_state, val);
> +}
> +
>  static u32 lpt_get_backlight(struct intel_connector *connector)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
> @@ -624,12 +633,12 @@ static void i9xx_set_backlight(const struct drm_connector_state *conn_state, u32
>  	struct intel_panel *panel = &connector->panel;
>  	u32 tmp, mask;
>  
> -	drm_WARN_ON(&dev_priv->drm, panel->backlight.max == 0);
> +	drm_WARN_ON(&dev_priv->drm, panel->backlight.pwm_max == 0);
>  
>  	if (panel->backlight.combination_mode) {
>  		u8 lbpc;
>  
> -		lbpc = level * 0xfe / panel->backlight.max + 1;
> +		lbpc = level * 0xfe / panel->backlight.pwm_max + 1;
>  		level /= lbpc;
>  		pci_write_config_byte(dev_priv->drm.pdev, LBPC, lbpc);
>  	}
> @@ -681,9 +690,8 @@ intel_panel_actually_set_backlight(const struct drm_connector_state *conn_state,
>  	struct drm_i915_private *i915 = to_i915(connector->base.dev);
>  	struct intel_panel *panel = &connector->panel;
>  
> -	drm_dbg_kms(&i915->drm, "set backlight PWM = %d\n", level);
> +	drm_dbg_kms(&i915->drm, "set backlight level = %d\n", level);
>  
> -	level = intel_panel_compute_brightness(connector, level);
>  	panel->backlight.set(conn_state, level);
>  }
>  
> @@ -726,13 +734,13 @@ void intel_panel_set_backlight_acpi(const struct drm_connector_state *conn_state
>  	mutex_unlock(&dev_priv->backlight_lock);
>  }
>  
> -static void lpt_disable_backlight(const struct drm_connector_state *old_conn_state)
> +static void lpt_disable_backlight(const struct drm_connector_state *old_conn_state, u32 level)
>  {
>  	struct intel_connector *connector = to_intel_connector(old_conn_state->connector);
>  	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
>  	u32 tmp;
>  
> -	intel_panel_actually_set_backlight(old_conn_state, 0);
> +	intel_panel_set_pwm_level(old_conn_state, level);
>  
>  	/*
>  	 * Although we don't support or enable CPU PWM with LPT/SPT based
> @@ -754,13 +762,13 @@ static void lpt_disable_backlight(const struct drm_connector_state *old_conn_sta
>  	intel_de_write(dev_priv, BLC_PWM_PCH_CTL1, tmp & ~BLM_PCH_PWM_ENABLE);
>  }
>  
> -static void pch_disable_backlight(const struct drm_connector_state *old_conn_state)
> +static void pch_disable_backlight(const struct drm_connector_state *old_conn_state, u32 val)
>  {
>  	struct intel_connector *connector = to_intel_connector(old_conn_state->connector);
>  	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
>  	u32 tmp;
>  
> -	intel_panel_actually_set_backlight(old_conn_state, 0);
> +	intel_panel_set_pwm_level(old_conn_state, val);
>  
>  	tmp = intel_de_read(dev_priv, BLC_PWM_CPU_CTL2);
>  	intel_de_write(dev_priv, BLC_PWM_CPU_CTL2, tmp & ~BLM_PWM_ENABLE);
> @@ -769,44 +777,44 @@ static void pch_disable_backlight(const struct drm_connector_state *old_conn_sta
>  	intel_de_write(dev_priv, BLC_PWM_PCH_CTL1, tmp & ~BLM_PCH_PWM_ENABLE);
>  }
>  
> -static void i9xx_disable_backlight(const struct drm_connector_state *old_conn_state)
> +static void i9xx_disable_backlight(const struct drm_connector_state *old_conn_state, u32 val)
>  {
> -	intel_panel_actually_set_backlight(old_conn_state, 0);
> +	intel_panel_set_pwm_level(old_conn_state, val);
>  }
>  
> -static void i965_disable_backlight(const struct drm_connector_state *old_conn_state)
> +static void i965_disable_backlight(const struct drm_connector_state *old_conn_state, u32 val)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(old_conn_state->connector->dev);
>  	u32 tmp;
>  
> -	intel_panel_actually_set_backlight(old_conn_state, 0);
> +	intel_panel_set_pwm_level(old_conn_state, val);
>  
>  	tmp = intel_de_read(dev_priv, BLC_PWM_CTL2);
>  	intel_de_write(dev_priv, BLC_PWM_CTL2, tmp & ~BLM_PWM_ENABLE);
>  }
>  
> -static void vlv_disable_backlight(const struct drm_connector_state *old_conn_state)
> +static void vlv_disable_backlight(const struct drm_connector_state *old_conn_state, u32 val)
>  {
>  	struct intel_connector *connector = to_intel_connector(old_conn_state->connector);
>  	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
>  	enum pipe pipe = to_intel_crtc(old_conn_state->crtc)->pipe;
>  	u32 tmp;
>  
> -	intel_panel_actually_set_backlight(old_conn_state, 0);
> +	intel_panel_set_pwm_level(old_conn_state, val);
>  
>  	tmp = intel_de_read(dev_priv, VLV_BLC_PWM_CTL2(pipe));
>  	intel_de_write(dev_priv, VLV_BLC_PWM_CTL2(pipe),
>  		       tmp & ~BLM_PWM_ENABLE);
>  }
>  
> -static void bxt_disable_backlight(const struct drm_connector_state *old_conn_state)
> +static void bxt_disable_backlight(const struct drm_connector_state *old_conn_state, u32 val)
>  {
>  	struct intel_connector *connector = to_intel_connector(old_conn_state->connector);
>  	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
>  	struct intel_panel *panel = &connector->panel;
> -	u32 tmp, val;
> +	u32 tmp;
>  
> -	intel_panel_actually_set_backlight(old_conn_state, 0);
> +	intel_panel_set_pwm_level(old_conn_state, val);
>  
>  	tmp = intel_de_read(dev_priv,
>  			    BXT_BLC_PWM_CTL(panel->backlight.controller));
> @@ -820,14 +828,14 @@ static void bxt_disable_backlight(const struct drm_connector_state *old_conn_sta
>  	}
>  }
>  
> -static void cnp_disable_backlight(const struct drm_connector_state *old_conn_state)
> +static void cnp_disable_backlight(const struct drm_connector_state *old_conn_state, u32 val)
>  {
>  	struct intel_connector *connector = to_intel_connector(old_conn_state->connector);
>  	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
>  	struct intel_panel *panel = &connector->panel;
>  	u32 tmp;
>  
> -	intel_panel_actually_set_backlight(old_conn_state, 0);
> +	intel_panel_set_pwm_level(old_conn_state, val);
>  
>  	tmp = intel_de_read(dev_priv,
>  			    BXT_BLC_PWM_CTL(panel->backlight.controller));
> @@ -835,7 +843,7 @@ static void cnp_disable_backlight(const struct drm_connector_state *old_conn_sta
>  		       tmp & ~BXT_BLC_PWM_ENABLE);
>  }
>  
> -static void ext_pwm_disable_backlight(const struct drm_connector_state *old_conn_state)
> +static void ext_pwm_disable_backlight(const struct drm_connector_state *old_conn_state, u32 level)
>  {
>  	struct intel_connector *connector = to_intel_connector(old_conn_state->connector);
>  	struct intel_panel *panel = &connector->panel;
> @@ -876,7 +884,7 @@ void intel_panel_disable_backlight(const struct drm_connector_state *old_conn_st
>  }
>  
>  static void lpt_enable_backlight(const struct intel_crtc_state *crtc_state,
> -				 const struct drm_connector_state *conn_state)
> +				 const struct drm_connector_state *conn_state, u32 level)
>  {
>  	struct intel_connector *connector = to_intel_connector(conn_state->connector);
>  	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
> @@ -906,7 +914,7 @@ static void lpt_enable_backlight(const struct intel_crtc_state *crtc_state,
>  		intel_de_write(dev_priv, SOUTH_CHICKEN1, schicken);
>  	}
>  
> -	pch_ctl2 = panel->backlight.max << 16;
> +	pch_ctl2 = panel->backlight.pwm_max << 16;
>  	intel_de_write(dev_priv, BLC_PWM_PCH_CTL2, pch_ctl2);
>  
>  	pch_ctl1 = 0;
> @@ -923,11 +931,11 @@ static void lpt_enable_backlight(const struct intel_crtc_state *crtc_state,
>  		       pch_ctl1 | BLM_PCH_PWM_ENABLE);
>  
>  	/* This won't stick until the above enable. */
> -	intel_panel_actually_set_backlight(conn_state, panel->backlight.level);
> +	intel_panel_set_pwm_level(conn_state, level);
>  }
>  
>  static void pch_enable_backlight(const struct intel_crtc_state *crtc_state,
> -				 const struct drm_connector_state *conn_state)
> +				 const struct drm_connector_state *conn_state, u32 level)
>  {
>  	struct intel_connector *connector = to_intel_connector(conn_state->connector);
>  	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
> @@ -958,9 +966,9 @@ static void pch_enable_backlight(const struct intel_crtc_state *crtc_state,
>  	intel_de_write(dev_priv, BLC_PWM_CPU_CTL2, cpu_ctl2 | BLM_PWM_ENABLE);
>  
>  	/* This won't stick until the above enable. */
> -	intel_panel_actually_set_backlight(conn_state, panel->backlight.level);
> +	intel_panel_set_pwm_level(conn_state, level);
>  
> -	pch_ctl2 = panel->backlight.max << 16;
> +	pch_ctl2 = panel->backlight.pwm_max << 16;
>  	intel_de_write(dev_priv, BLC_PWM_PCH_CTL2, pch_ctl2);
>  
>  	pch_ctl1 = 0;
> @@ -974,7 +982,7 @@ static void pch_enable_backlight(const struct intel_crtc_state *crtc_state,
>  }
>  
>  static void i9xx_enable_backlight(const struct intel_crtc_state *crtc_state,
> -				  const struct drm_connector_state *conn_state)
> +				  const struct drm_connector_state *conn_state, u32 level)
>  {
>  	struct intel_connector *connector = to_intel_connector(conn_state->connector);
>  	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
> @@ -987,7 +995,7 @@ static void i9xx_enable_backlight(const struct intel_crtc_state *crtc_state,
>  		intel_de_write(dev_priv, BLC_PWM_CTL, 0);
>  	}
>  
> -	freq = panel->backlight.max;
> +	freq = panel->backlight.pwm_max;
>  	if (panel->backlight.combination_mode)
>  		freq /= 0xff;
>  
> @@ -1001,7 +1009,7 @@ static void i9xx_enable_backlight(const struct intel_crtc_state *crtc_state,
>  	intel_de_posting_read(dev_priv, BLC_PWM_CTL);
>  
>  	/* XXX: combine this into above write? */
> -	intel_panel_actually_set_backlight(conn_state, panel->backlight.level);
> +	intel_panel_set_pwm_level(conn_state, level);
>  
>  	/*
>  	 * Needed to enable backlight on some 855gm models. BLC_HIST_CTL is
> @@ -1013,7 +1021,7 @@ static void i9xx_enable_backlight(const struct intel_crtc_state *crtc_state,
>  }
>  
>  static void i965_enable_backlight(const struct intel_crtc_state *crtc_state,
> -				  const struct drm_connector_state *conn_state)
> +				  const struct drm_connector_state *conn_state, u32 level)
>  {
>  	struct intel_connector *connector = to_intel_connector(conn_state->connector);
>  	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
> @@ -1028,7 +1036,7 @@ static void i965_enable_backlight(const struct intel_crtc_state *crtc_state,
>  		intel_de_write(dev_priv, BLC_PWM_CTL2, ctl2);
>  	}
>  
> -	freq = panel->backlight.max;
> +	freq = panel->backlight.pwm_max;
>  	if (panel->backlight.combination_mode)
>  		freq /= 0xff;
>  
> @@ -1044,11 +1052,11 @@ static void i965_enable_backlight(const struct intel_crtc_state *crtc_state,
>  	intel_de_posting_read(dev_priv, BLC_PWM_CTL2);
>  	intel_de_write(dev_priv, BLC_PWM_CTL2, ctl2 | BLM_PWM_ENABLE);
>  
> -	intel_panel_actually_set_backlight(conn_state, panel->backlight.level);
> +	intel_panel_set_pwm_level(conn_state, level);
>  }
>  
>  static void vlv_enable_backlight(const struct intel_crtc_state *crtc_state,
> -				 const struct drm_connector_state *conn_state)
> +				 const struct drm_connector_state *conn_state, u32 level)
>  {
>  	struct intel_connector *connector = to_intel_connector(conn_state->connector);
>  	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
> @@ -1063,11 +1071,11 @@ static void vlv_enable_backlight(const struct intel_crtc_state *crtc_state,
>  		intel_de_write(dev_priv, VLV_BLC_PWM_CTL2(pipe), ctl2);
>  	}
>  
> -	ctl = panel->backlight.max << 16;
> +	ctl = panel->backlight.pwm_max << 16;
>  	intel_de_write(dev_priv, VLV_BLC_PWM_CTL(pipe), ctl);
>  
>  	/* XXX: combine this into above write? */
> -	intel_panel_actually_set_backlight(conn_state, panel->backlight.level);
> +	intel_panel_set_pwm_level(conn_state, level);
>  
>  	ctl2 = 0;
>  	if (panel->backlight.active_low_pwm)
> @@ -1079,7 +1087,7 @@ static void vlv_enable_backlight(const struct intel_crtc_state *crtc_state,
>  }
>  
>  static void bxt_enable_backlight(const struct intel_crtc_state *crtc_state,
> -				 const struct drm_connector_state *conn_state)
> +				 const struct drm_connector_state *conn_state, u32 level)
>  {
>  	struct intel_connector *connector = to_intel_connector(conn_state->connector);
>  	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
> @@ -1116,9 +1124,9 @@ static void bxt_enable_backlight(const struct intel_crtc_state *crtc_state,
>  
>  	intel_de_write(dev_priv,
>  		       BXT_BLC_PWM_FREQ(panel->backlight.controller),
> -		       panel->backlight.max);
> +		       panel->backlight.pwm_max);
>  
> -	intel_panel_actually_set_backlight(conn_state, panel->backlight.level);
> +	intel_panel_set_pwm_level(conn_state, level);
>  
>  	pwm_ctl = 0;
>  	if (panel->backlight.active_low_pwm)
> @@ -1133,7 +1141,7 @@ static void bxt_enable_backlight(const struct intel_crtc_state *crtc_state,
>  }
>  
>  static void cnp_enable_backlight(const struct intel_crtc_state *crtc_state,
> -				 const struct drm_connector_state *conn_state)
> +				 const struct drm_connector_state *conn_state, u32 level)
>  {
>  	struct intel_connector *connector = to_intel_connector(conn_state->connector);
>  	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
> @@ -1152,9 +1160,9 @@ static void cnp_enable_backlight(const struct intel_crtc_state *crtc_state,
>  
>  	intel_de_write(dev_priv,
>  		       BXT_BLC_PWM_FREQ(panel->backlight.controller),
> -		       panel->backlight.max);
> +		       panel->backlight.pwm_max);
>  
> -	intel_panel_actually_set_backlight(conn_state, panel->backlight.level);
> +	intel_panel_set_pwm_level(conn_state, level);
>  
>  	pwm_ctl = 0;
>  	if (panel->backlight.active_low_pwm)
> @@ -1169,13 +1177,11 @@ static void cnp_enable_backlight(const struct intel_crtc_state *crtc_state,
>  }
>  
>  static void ext_pwm_enable_backlight(const struct intel_crtc_state *crtc_state,
> -				     const struct drm_connector_state *conn_state)
> +				     const struct drm_connector_state *conn_state, u32 level)
>  {
>  	struct intel_connector *connector = to_intel_connector(conn_state->connector);
>  	struct intel_panel *panel = &connector->panel;
> -	int level = panel->backlight.level;
>  
> -	level = intel_panel_compute_brightness(connector, level);
>  	pwm_set_relative_duty_cycle(&panel->backlight.pwm_state, level, 100);
>  	panel->backlight.pwm_state.enabled = true;
>  	pwm_apply_state(panel->backlight.pwm, &panel->backlight.pwm_state);
> @@ -1233,10 +1239,8 @@ static u32 intel_panel_get_backlight(struct intel_connector *connector)
>  
>  	mutex_lock(&dev_priv->backlight_lock);
>  
> -	if (panel->backlight.enabled) {
> +	if (panel->backlight.enabled)
>  		val = panel->backlight.get(connector);
> -		val = intel_panel_compute_brightness(connector, val);
> -	}
>  
>  	mutex_unlock(&dev_priv->backlight_lock);
>  
> @@ -1567,13 +1571,13 @@ static u32 get_backlight_max_vbt(struct intel_connector *connector)
>  	u16 pwm_freq_hz = get_vbt_pwm_freq(dev_priv);
>  	u32 pwm;
>  
> -	if (!panel->backlight.hz_to_pwm) {
> +	if (!panel->backlight.pwm_funcs.hz_to_pwm) {
>  		drm_dbg_kms(&dev_priv->drm,
>  			    "backlight frequency conversion not supported\n");
>  		return 0;
>  	}
>  
> -	pwm = panel->backlight.hz_to_pwm(connector, pwm_freq_hz);
> +	pwm = panel->backlight.pwm_funcs.hz_to_pwm(connector, pwm_freq_hz);
>  	if (!pwm) {
>  		drm_dbg_kms(&dev_priv->drm,
>  			    "backlight frequency conversion failed\n");
> @@ -1592,7 +1596,7 @@ static u32 get_backlight_min_vbt(struct intel_connector *connector)
>  	struct intel_panel *panel = &connector->panel;
>  	int min;
>  
> -	drm_WARN_ON(&dev_priv->drm, panel->backlight.max == 0);
> +	drm_WARN_ON(&dev_priv->drm, panel->backlight.pwm_max == 0);
>  
>  	/*
>  	 * XXX: If the vbt value is 255, it makes min equal to max, which leads
> @@ -1609,7 +1613,7 @@ static u32 get_backlight_min_vbt(struct intel_connector *connector)
>  	}
>  
>  	/* vbt value is a coefficient in range [0..255] */
> -	return scale(min, 0, 255, 0, panel->backlight.max);
> +	return scale(min, 0, 255, 0, panel->backlight.pwm_max);
>  }
>  
>  static int lpt_setup_backlight(struct intel_connector *connector, enum pipe unused)
> @@ -1629,37 +1633,32 @@ static int lpt_setup_backlight(struct intel_connector *connector, enum pipe unus
>  	panel->backlight.active_low_pwm = pch_ctl1 & BLM_PCH_POLARITY;
>  
>  	pch_ctl2 = intel_de_read(dev_priv, BLC_PWM_PCH_CTL2);
> -	panel->backlight.max = pch_ctl2 >> 16;
> +	panel->backlight.pwm_max = pch_ctl2 >> 16;
>  
>  	cpu_ctl2 = intel_de_read(dev_priv, BLC_PWM_CPU_CTL2);
>  
> -	if (!panel->backlight.max)
> -		panel->backlight.max = get_backlight_max_vbt(connector);
> +	if (!panel->backlight.pwm_max)
> +		panel->backlight.pwm_max = get_backlight_max_vbt(connector);
>  
> -	if (!panel->backlight.max)
> +	if (!panel->backlight.pwm_max)
>  		return -ENODEV;
>  
> -	panel->backlight.min = get_backlight_min_vbt(connector);
> +	panel->backlight.pwm_min = get_backlight_min_vbt(connector);
>  
> -	panel->backlight.enabled = pch_ctl1 & BLM_PCH_PWM_ENABLE;
> +	panel->backlight.pwm_enabled = pch_ctl1 & BLM_PCH_PWM_ENABLE;
>  
> -	cpu_mode = panel->backlight.enabled && HAS_PCH_LPT(dev_priv) &&
> +	cpu_mode = panel->backlight.pwm_enabled && HAS_PCH_LPT(dev_priv) &&
>  		   !(pch_ctl1 & BLM_PCH_OVERRIDE_ENABLE) &&
>  		   (cpu_ctl2 & BLM_PWM_ENABLE);
> -	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);
>  
> @@ -1674,29 +1673,24 @@ static int pch_setup_backlight(struct intel_connector *connector, enum pipe unus
>  {
>  	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
>  	struct intel_panel *panel = &connector->panel;
> -	u32 cpu_ctl2, pch_ctl1, pch_ctl2, val;
> +	u32 cpu_ctl2, pch_ctl1, pch_ctl2;
>  
>  	pch_ctl1 = intel_de_read(dev_priv, BLC_PWM_PCH_CTL1);
>  	panel->backlight.active_low_pwm = pch_ctl1 & BLM_PCH_POLARITY;
>  
>  	pch_ctl2 = intel_de_read(dev_priv, BLC_PWM_PCH_CTL2);
> -	panel->backlight.max = pch_ctl2 >> 16;
> +	panel->backlight.pwm_max = pch_ctl2 >> 16;
>  
> -	if (!panel->backlight.max)
> -		panel->backlight.max = get_backlight_max_vbt(connector);
> +	if (!panel->backlight.pwm_max)
> +		panel->backlight.pwm_max = get_backlight_max_vbt(connector);
>  
> -	if (!panel->backlight.max)
> +	if (!panel->backlight.pwm_max)
>  		return -ENODEV;
>  
> -	panel->backlight.min = get_backlight_min_vbt(connector);
> -
> -	val = pch_get_backlight(connector);
> -	val = intel_panel_compute_brightness(connector, val);
> -	panel->backlight.level = clamp(val, panel->backlight.min,
> -				       panel->backlight.max);
> +	panel->backlight.pwm_min = get_backlight_min_vbt(connector);
>  
>  	cpu_ctl2 = intel_de_read(dev_priv, BLC_PWM_CPU_CTL2);
> -	panel->backlight.enabled = (cpu_ctl2 & BLM_PWM_ENABLE) &&
> +	panel->backlight.pwm_enabled = (cpu_ctl2 & BLM_PWM_ENABLE) &&
>  		(pch_ctl1 & BLM_PCH_PWM_ENABLE);
>  
>  	return 0;
> @@ -1716,27 +1710,26 @@ static int i9xx_setup_backlight(struct intel_connector *connector, enum pipe unu
>  	if (IS_PINEVIEW(dev_priv))
>  		panel->backlight.active_low_pwm = ctl & BLM_POLARITY_PNV;
>  
> -	panel->backlight.max = ctl >> 17;
> +	panel->backlight.pwm_max = ctl >> 17;
>  
> -	if (!panel->backlight.max) {
> -		panel->backlight.max = get_backlight_max_vbt(connector);
> -		panel->backlight.max >>= 1;
> +	if (!panel->backlight.pwm_max) {
> +		panel->backlight.pwm_max = get_backlight_max_vbt(connector);
> +		panel->backlight.pwm_max >>= 1;
>  	}
>  
> -	if (!panel->backlight.max)
> +	if (!panel->backlight.pwm_max)
>  		return -ENODEV;
>  
>  	if (panel->backlight.combination_mode)
> -		panel->backlight.max *= 0xff;
> +		panel->backlight.pwm_max *= 0xff;
>  
> -	panel->backlight.min = get_backlight_min_vbt(connector);
> +	panel->backlight.pwm_min = get_backlight_min_vbt(connector);
>  
>  	val = i9xx_get_backlight(connector);
> -	val = intel_panel_compute_brightness(connector, val);
> -	panel->backlight.level = clamp(val, panel->backlight.min,
> -				       panel->backlight.max);
> +	val = intel_panel_sanitize_pwm_level(connector, val);
> +	val = clamp(val, panel->backlight.pwm_min, panel->backlight.pwm_max);
>  
> -	panel->backlight.enabled = val != 0;
> +	panel->backlight.pwm_enabled = val != 0;
>  
>  	return 0;
>  }
> @@ -1745,32 +1738,27 @@ static int i965_setup_backlight(struct intel_connector *connector, enum pipe unu
>  {
>  	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
>  	struct intel_panel *panel = &connector->panel;
> -	u32 ctl, ctl2, val;
> +	u32 ctl, ctl2;
>  
>  	ctl2 = intel_de_read(dev_priv, BLC_PWM_CTL2);
>  	panel->backlight.combination_mode = ctl2 & BLM_COMBINATION_MODE;
>  	panel->backlight.active_low_pwm = ctl2 & BLM_POLARITY_I965;
>  
>  	ctl = intel_de_read(dev_priv, BLC_PWM_CTL);
> -	panel->backlight.max = ctl >> 16;
> +	panel->backlight.pwm_max = ctl >> 16;
>  
> -	if (!panel->backlight.max)
> -		panel->backlight.max = get_backlight_max_vbt(connector);
> +	if (!panel->backlight.pwm_max)
> +		panel->backlight.pwm_max = get_backlight_max_vbt(connector);
>  
> -	if (!panel->backlight.max)
> +	if (!panel->backlight.pwm_max)
>  		return -ENODEV;
>  
>  	if (panel->backlight.combination_mode)
> -		panel->backlight.max *= 0xff;
> +		panel->backlight.pwm_max *= 0xff;
>  
> -	panel->backlight.min = get_backlight_min_vbt(connector);
> +	panel->backlight.pwm_min = get_backlight_min_vbt(connector);
>  
> -	val = i9xx_get_backlight(connector);
> -	val = intel_panel_compute_brightness(connector, val);
> -	panel->backlight.level = clamp(val, panel->backlight.min,
> -				       panel->backlight.max);
> -
> -	panel->backlight.enabled = ctl2 & BLM_PWM_ENABLE;
> +	panel->backlight.pwm_enabled = ctl2 & BLM_PWM_ENABLE;
>  
>  	return 0;
>  }
> @@ -1779,7 +1767,7 @@ static int vlv_setup_backlight(struct intel_connector *connector, enum pipe pipe
>  {
>  	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
>  	struct intel_panel *panel = &connector->panel;
> -	u32 ctl, ctl2, val;
> +	u32 ctl, ctl2;
>  
>  	if (drm_WARN_ON(&dev_priv->drm, pipe != PIPE_A && pipe != PIPE_B))
>  		return -ENODEV;
> @@ -1788,22 +1776,17 @@ static int vlv_setup_backlight(struct intel_connector *connector, enum pipe pipe
>  	panel->backlight.active_low_pwm = ctl2 & BLM_POLARITY_I965;
>  
>  	ctl = intel_de_read(dev_priv, VLV_BLC_PWM_CTL(pipe));
> -	panel->backlight.max = ctl >> 16;
> +	panel->backlight.pwm_max = ctl >> 16;
>  
> -	if (!panel->backlight.max)
> -		panel->backlight.max = get_backlight_max_vbt(connector);
> +	if (!panel->backlight.pwm_max)
> +		panel->backlight.pwm_max = get_backlight_max_vbt(connector);
>  
> -	if (!panel->backlight.max)
> +	if (!panel->backlight.pwm_max)
>  		return -ENODEV;
>  
> -	panel->backlight.min = get_backlight_min_vbt(connector);
> +	panel->backlight.pwm_min = get_backlight_min_vbt(connector);
>  
> -	val = _vlv_get_backlight(dev_priv, pipe);
> -	val = intel_panel_compute_brightness(connector, val);
> -	panel->backlight.level = clamp(val, panel->backlight.min,
> -				       panel->backlight.max);
> -
> -	panel->backlight.enabled = ctl2 & BLM_PWM_ENABLE;
> +	panel->backlight.pwm_enabled = ctl2 & BLM_PWM_ENABLE;
>  
>  	return 0;
>  }
> @@ -1828,24 +1811,18 @@ bxt_setup_backlight(struct intel_connector *connector, enum pipe unused)
>  	}
>  
>  	panel->backlight.active_low_pwm = pwm_ctl & BXT_BLC_PWM_POLARITY;
> -	panel->backlight.max =
> -		intel_de_read(dev_priv,
> -			      BXT_BLC_PWM_FREQ(panel->backlight.controller));
> +	panel->backlight.pwm_max = intel_de_read(dev_priv,
> +						 BXT_BLC_PWM_FREQ(panel->backlight.controller));
>  
> -	if (!panel->backlight.max)
> -		panel->backlight.max = get_backlight_max_vbt(connector);
> +	if (!panel->backlight.pwm_max)
> +		panel->backlight.pwm_max = get_backlight_max_vbt(connector);
>  
> -	if (!panel->backlight.max)
> +	if (!panel->backlight.pwm_max)
>  		return -ENODEV;
>  
> -	panel->backlight.min = get_backlight_min_vbt(connector);
> -
> -	val = bxt_get_backlight(connector);
> -	val = intel_panel_compute_brightness(connector, val);
> -	panel->backlight.level = clamp(val, panel->backlight.min,
> -				       panel->backlight.max);
> +	panel->backlight.pwm_min = get_backlight_min_vbt(connector);
>  
> -	panel->backlight.enabled = pwm_ctl & BXT_BLC_PWM_ENABLE;
> +	panel->backlight.pwm_enabled = pwm_ctl & BXT_BLC_PWM_ENABLE;
>  
>  	return 0;
>  }
> @@ -1855,7 +1832,7 @@ cnp_setup_backlight(struct intel_connector *connector, enum pipe unused)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
>  	struct intel_panel *panel = &connector->panel;
> -	u32 pwm_ctl, val;
> +	u32 pwm_ctl;
>  
>  	/*
>  	 * CNP has the BXT implementation of backlight, but with only one
> @@ -1868,24 +1845,18 @@ cnp_setup_backlight(struct intel_connector *connector, enum pipe unused)
>  				BXT_BLC_PWM_CTL(panel->backlight.controller));
>  
>  	panel->backlight.active_low_pwm = pwm_ctl & BXT_BLC_PWM_POLARITY;
> -	panel->backlight.max =
> -		intel_de_read(dev_priv,
> -			      BXT_BLC_PWM_FREQ(panel->backlight.controller));
> +	panel->backlight.pwm_max = intel_de_read(dev_priv,
> +						 BXT_BLC_PWM_FREQ(panel->backlight.controller));
>  
> -	if (!panel->backlight.max)
> -		panel->backlight.max = get_backlight_max_vbt(connector);
> +	if (!panel->backlight.pwm_max)
> +		panel->backlight.pwm_max = get_backlight_max_vbt(connector);
>  
> -	if (!panel->backlight.max)
> +	if (!panel->backlight.pwm_max)
>  		return -ENODEV;
>  
> -	panel->backlight.min = get_backlight_min_vbt(connector);
> -
> -	val = bxt_get_backlight(connector);
> -	val = intel_panel_compute_brightness(connector, val);
> -	panel->backlight.level = clamp(val, panel->backlight.min,
> -				       panel->backlight.max);
> +	panel->backlight.pwm_min = get_backlight_min_vbt(connector);
>  
> -	panel->backlight.enabled = pwm_ctl & BXT_BLC_PWM_ENABLE;
> +	panel->backlight.pwm_enabled = pwm_ctl & BXT_BLC_PWM_ENABLE;
>  
>  	return 0;
>  }
> @@ -1915,8 +1886,8 @@ static int ext_pwm_setup_backlight(struct intel_connector *connector,
>  		return -ENODEV;
>  	}
>  
> -	panel->backlight.max = 100; /* 100% */
> -	panel->backlight.min = get_backlight_min_vbt(connector);
> +	panel->backlight.pwm_max = 100; /* 100% */
> +	panel->backlight.pwm_min = get_backlight_min_vbt(connector);
>  
>  	if (pwm_is_enabled(panel->backlight.pwm)) {
>  		/* PWM is already enabled, use existing settings */
> @@ -1924,10 +1895,8 @@ static int ext_pwm_setup_backlight(struct intel_connector *connector,
>  
>  		level = pwm_get_relative_duty_cycle(&panel->backlight.pwm_state,
>  						    100);
> -		level = intel_panel_compute_brightness(connector, level);
> -		panel->backlight.level = clamp(level, panel->backlight.min,
> -					       panel->backlight.max);
> -		panel->backlight.enabled = true;
> +		level = intel_panel_sanitize_pwm_level(connector, level);
> +		panel->backlight.pwm_enabled = true;
>  
>  		drm_dbg_kms(&dev_priv->drm, "PWM already enabled at freq %ld, VBT freq %d, level %d\n",
>  			    NSEC_PER_SEC / (unsigned long)panel->backlight.pwm_state.period,
> @@ -1943,6 +1912,60 @@ static int ext_pwm_setup_backlight(struct intel_connector *connector,
>  	return 0;
>  }
>  
> +static void intel_pwm_set_backlight(const struct drm_connector_state *conn_state, u32 level)
> +{
> +	struct intel_connector *connector = to_intel_connector(conn_state->connector);
> +	struct intel_panel *panel = &connector->panel;
> +
> +	panel->backlight.pwm_funcs.set(conn_state,
> +				       intel_panel_sanitize_pwm_level(connector, level));
> +}
> +
> +static u32 intel_pwm_get_backlight(struct intel_connector *connector)
> +{
> +	struct intel_panel *panel = &connector->panel;
> +
> +	return intel_panel_sanitize_pwm_level(connector, panel->backlight.pwm_funcs.get(connector));
> +}
> +
> +static void intel_pwm_enable_backlight(const struct intel_crtc_state *crtc_state,
> +				       const struct drm_connector_state *conn_state)
> +{
> +	struct intel_connector *connector = to_intel_connector(conn_state->connector);
> +	struct intel_panel *panel = &connector->panel;
> +	u32 level = intel_panel_sanitize_pwm_level(connector, panel->backlight.level);
> +
> +	panel->backlight.pwm_funcs.enable(crtc_state, conn_state, level);
> +}
> +
> +static void intel_pwm_disable_backlight(const struct drm_connector_state *conn_state)
> +{
> +	struct intel_connector *connector = to_intel_connector(conn_state->connector);
> +	struct intel_panel *panel = &connector->panel;
> +
> +	panel->backlight.pwm_funcs.disable(conn_state,
> +					   intel_panel_sanitize_pwm_level(connector, 0));
> +}
> +
> +/* The only hook PWM-only backlight setups need to override, the rest of the hooks are filled in
> + * from pwm_funcs
> + */
> +static int intel_pwm_setup_backlight(struct intel_connector *connector, enum pipe pipe)
> +{
> +	struct intel_panel *panel = &connector->panel;
> +	int ret = panel->backlight.pwm_funcs.setup(connector, pipe);
> +
> +	if (ret < 0)
> +		return ret;
> +
> +	panel->backlight.min = panel->backlight.pwm_min;
> +	panel->backlight.max = panel->backlight.pwm_max;
> +	panel->backlight.level = panel->backlight.get(connector);
> +	panel->backlight.enabled = panel->backlight.pwm_enabled;
> +
> +	return 0;
> +}
> +
>  void intel_panel_update_backlight(struct intel_atomic_state *state,
>  				  struct intel_encoder *encoder,
>  				  const struct intel_crtc_state *crtc_state,
> @@ -2024,75 +2047,82 @@ intel_panel_init_backlight_funcs(struct intel_panel *panel)
>  		container_of(panel, struct intel_connector, panel);
>  	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
>  
> -	if (connector->base.connector_type == DRM_MODE_CONNECTOR_eDP &&
> -	    intel_dp_aux_init_backlight_funcs(connector) == 0)
> -		return;
> -
>  	if (connector->base.connector_type == DRM_MODE_CONNECTOR_DSI &&
>  	    intel_dsi_dcs_init_backlight_funcs(connector) == 0)
>  		return;
>  
>  	if (IS_GEN9_LP(dev_priv)) {
> -		panel->backlight.setup = bxt_setup_backlight;
> -		panel->backlight.enable = bxt_enable_backlight;
> -		panel->backlight.disable = bxt_disable_backlight;
> -		panel->backlight.set = bxt_set_backlight;
> -		panel->backlight.get = bxt_get_backlight;
> -		panel->backlight.hz_to_pwm = bxt_hz_to_pwm;
> +		panel->backlight.pwm_funcs.setup = bxt_setup_backlight;
> +		panel->backlight.pwm_funcs.enable = bxt_enable_backlight;
> +		panel->backlight.pwm_funcs.disable = bxt_disable_backlight;
> +		panel->backlight.pwm_funcs.set = bxt_set_backlight;
> +		panel->backlight.pwm_funcs.get = bxt_get_backlight;
> +		panel->backlight.pwm_funcs.hz_to_pwm = bxt_hz_to_pwm;
>  	} else if (INTEL_PCH_TYPE(dev_priv) >= PCH_CNP) {
> -		panel->backlight.setup = cnp_setup_backlight;
> -		panel->backlight.enable = cnp_enable_backlight;
> -		panel->backlight.disable = cnp_disable_backlight;
> -		panel->backlight.set = bxt_set_backlight;
> -		panel->backlight.get = bxt_get_backlight;
> -		panel->backlight.hz_to_pwm = cnp_hz_to_pwm;
> +		panel->backlight.pwm_funcs.setup = cnp_setup_backlight;
> +		panel->backlight.pwm_funcs.enable = cnp_enable_backlight;
> +		panel->backlight.pwm_funcs.disable = cnp_disable_backlight;
> +		panel->backlight.pwm_funcs.set = bxt_set_backlight;
> +		panel->backlight.pwm_funcs.get = bxt_get_backlight;
> +		panel->backlight.pwm_funcs.hz_to_pwm = cnp_hz_to_pwm;
>  	} else if (INTEL_PCH_TYPE(dev_priv) >= PCH_LPT) {
> -		panel->backlight.setup = lpt_setup_backlight;
> -		panel->backlight.enable = lpt_enable_backlight;
> -		panel->backlight.disable = lpt_disable_backlight;
> -		panel->backlight.set = lpt_set_backlight;
> -		panel->backlight.get = lpt_get_backlight;
> +		panel->backlight.pwm_funcs.setup = lpt_setup_backlight;
> +		panel->backlight.pwm_funcs.enable = lpt_enable_backlight;
> +		panel->backlight.pwm_funcs.disable = lpt_disable_backlight;
> +		panel->backlight.pwm_funcs.set = lpt_set_backlight;
> +		panel->backlight.pwm_funcs.get = lpt_get_backlight;
>  		if (HAS_PCH_LPT(dev_priv))
> -			panel->backlight.hz_to_pwm = lpt_hz_to_pwm;
> +			panel->backlight.pwm_funcs.hz_to_pwm = lpt_hz_to_pwm;
>  		else
> -			panel->backlight.hz_to_pwm = spt_hz_to_pwm;
> +			panel->backlight.pwm_funcs.hz_to_pwm = spt_hz_to_pwm;
>  	} else if (HAS_PCH_SPLIT(dev_priv)) {
> -		panel->backlight.setup = pch_setup_backlight;
> -		panel->backlight.enable = pch_enable_backlight;
> -		panel->backlight.disable = pch_disable_backlight;
> -		panel->backlight.set = pch_set_backlight;
> -		panel->backlight.get = pch_get_backlight;
> -		panel->backlight.hz_to_pwm = pch_hz_to_pwm;
> +		panel->backlight.pwm_funcs.setup = pch_setup_backlight;
> +		panel->backlight.pwm_funcs.enable = pch_enable_backlight;
> +		panel->backlight.pwm_funcs.disable = pch_disable_backlight;
> +		panel->backlight.pwm_funcs.set = pch_set_backlight;
> +		panel->backlight.pwm_funcs.get = pch_get_backlight;
> +		panel->backlight.pwm_funcs.hz_to_pwm = pch_hz_to_pwm;
>  	} else if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
>  		if (connector->base.connector_type == DRM_MODE_CONNECTOR_DSI) {
> -			panel->backlight.setup = ext_pwm_setup_backlight;
> -			panel->backlight.enable = ext_pwm_enable_backlight;
> -			panel->backlight.disable = ext_pwm_disable_backlight;
> -			panel->backlight.set = ext_pwm_set_backlight;
> -			panel->backlight.get = ext_pwm_get_backlight;
> +			panel->backlight.pwm_funcs.setup = ext_pwm_setup_backlight;
> +			panel->backlight.pwm_funcs.enable = ext_pwm_enable_backlight;
> +			panel->backlight.pwm_funcs.disable = ext_pwm_disable_backlight;
> +			panel->backlight.pwm_funcs.set = ext_pwm_set_backlight;
> +			panel->backlight.pwm_funcs.get = ext_pwm_get_backlight;
>  		} else {
> -			panel->backlight.setup = vlv_setup_backlight;
> -			panel->backlight.enable = vlv_enable_backlight;
> -			panel->backlight.disable = vlv_disable_backlight;
> -			panel->backlight.set = vlv_set_backlight;
> -			panel->backlight.get = vlv_get_backlight;
> -			panel->backlight.hz_to_pwm = vlv_hz_to_pwm;
> +			panel->backlight.pwm_funcs.setup = vlv_setup_backlight;
> +			panel->backlight.pwm_funcs.enable = vlv_enable_backlight;
> +			panel->backlight.pwm_funcs.disable = vlv_disable_backlight;
> +			panel->backlight.pwm_funcs.set = vlv_set_backlight;
> +			panel->backlight.pwm_funcs.get = vlv_get_backlight;
> +			panel->backlight.pwm_funcs.hz_to_pwm = vlv_hz_to_pwm;
>  		}
>  	} else if (IS_GEN(dev_priv, 4)) {
> -		panel->backlight.setup = i965_setup_backlight;
> -		panel->backlight.enable = i965_enable_backlight;
> -		panel->backlight.disable = i965_disable_backlight;
> -		panel->backlight.set = i9xx_set_backlight;
> -		panel->backlight.get = i9xx_get_backlight;
> -		panel->backlight.hz_to_pwm = i965_hz_to_pwm;
> +		panel->backlight.pwm_funcs.setup = i965_setup_backlight;
> +		panel->backlight.pwm_funcs.enable = i965_enable_backlight;
> +		panel->backlight.pwm_funcs.disable = i965_disable_backlight;
> +		panel->backlight.pwm_funcs.set = i9xx_set_backlight;
> +		panel->backlight.pwm_funcs.get = i9xx_get_backlight;
> +		panel->backlight.pwm_funcs.hz_to_pwm = i965_hz_to_pwm;
>  	} else {
> -		panel->backlight.setup = i9xx_setup_backlight;
> -		panel->backlight.enable = i9xx_enable_backlight;
> -		panel->backlight.disable = i9xx_disable_backlight;
> -		panel->backlight.set = i9xx_set_backlight;
> -		panel->backlight.get = i9xx_get_backlight;
> -		panel->backlight.hz_to_pwm = i9xx_hz_to_pwm;
> +		panel->backlight.pwm_funcs.setup = i9xx_setup_backlight;
> +		panel->backlight.pwm_funcs.enable = i9xx_enable_backlight;
> +		panel->backlight.pwm_funcs.disable = i9xx_disable_backlight;
> +		panel->backlight.pwm_funcs.set = i9xx_set_backlight;
> +		panel->backlight.pwm_funcs.get = i9xx_get_backlight;
> +		panel->backlight.pwm_funcs.hz_to_pwm = i9xx_hz_to_pwm;
>  	}
> +
> +	if (connector->base.connector_type == DRM_MODE_CONNECTOR_eDP &&
> +	    intel_dp_aux_init_backlight_funcs(connector) == 0)
> +		return;
> +
> +	/* We're using a standard PWM backlight interface */
> +	panel->backlight.setup = intel_pwm_setup_backlight;
> +	panel->backlight.enable = intel_pwm_enable_backlight;
> +	panel->backlight.disable = intel_pwm_disable_backlight;
> +	panel->backlight.set = intel_pwm_set_backlight;
> +	panel->backlight.get = intel_pwm_get_backlight;
>  }
>  
>  enum drm_connector_status
> -- 
> 2.26.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [RFC v2 3/8] drm/i915: Keep track of pwm-related backlight hooks separately

[Dave Airlie]

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.

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.

Dave.

Re: [Intel-gfx] [RFC v2 3/8] drm/i915: Keep track of pwm-related backlight hooks separately

[Jani Nikula]

On Thu, 26 Nov 2020, Dave Airlie <airlied@gmail.com> 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.

Agreed. And not looking into and requesting this earlier is on me.

The thing that still keeps bugging me about the DPCD brightness control
in general is that it's a historical mistake to put all of this under
i915. (Again, mea culpa.) The standard DPCD brightness control should
really be under drm core, in one form or another.

I'm not asking to fix that here. But I *am* wondering if the series
makes that harder. What would it look like if we did have that unicorn
of a brightness connector property? How would that tie into the hooks we
have?

Maybe the answer is that the DPCD backlight functions should just be
library code in drm core that the drivers could call. In the long run,
i915 really can't be the only one needing this stuff.

We haven't implemented the mixed modes of DPCD and eDP PWM pin
brightness control. But the point is, the library code can't call into
i915 specific eDP PWM pin control code. The proprietary HDR brightness
code will still be i915 specific, but does it make sense to have a mixed
mode there that will be completely different from what a mixed mode with
the standard VESA DPCD brightness could be?

I.e. what should be the entry points for the hooks, and who calls what?

BR,
Jani.

>
> 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.
>
> Dave.
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Graphics Center

Re: [Intel-gfx] [RFC v2 3/8] drm/i915: Keep track of pwm-related backlight hooks separately

[Lyude Paul]

On Thu, 2020-11-26 at 13:57 +0200, Jani Nikula wrote:
> On Thu, 26 Nov 2020, Dave Airlie <airlied@gmail.com> 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.
> 
> Agreed. And not looking into and requesting this earlier is on me.
> 
> The thing that still keeps bugging me about the DPCD brightness control
> in general is that it's a historical mistake to put all of this under
> i915. (Again, mea culpa.) The standard DPCD brightness control should
> really be under drm core, in one form or another.

JFYI - I already actually have a WIP series to move all of the VESA standard
brightness stuff into the DRM core (especially since I am adding support for
the VESA interface to nouveau). It is pretty important to do so especially
considering some of the ways panel manufacturers seem to have consistently
gotten some portions of the spec wrong (there's currently a bug on almost
every panel I've run into, minus some panels in laptops that run ChromeOS,
where they interpret the brightness value as LSB aligned and not MSB aligned
(which is what the eDP spec actually says), because almost everyone misread
it. So, definitely the kind of stuff we'd want to keep in the drm core to make
maintaining quirks like this easier.

> 
> I'm not asking to fix that here. But I *am* wondering if the series
> makes that harder. What would it look like if we did have that unicorn
> of a brightness connector property? How would that tie into the hooks we
> have?

Re: making it harder, not really. But either way I'm planning on doing the
work for this anyway :)

> 
> Maybe the answer is that the DPCD backlight functions should just be
> library code in drm core that the drivers could call. In the long run,
> i915 really can't be the only one needing this stuff.
> 
> We haven't implemented the mixed modes of DPCD and eDP PWM pin
> brightness control. But the point is, the library code can't call into
> i915 specific eDP PWM pin control code. The proprietary HDR brightness
> code will still be i915 specific, but does it make sense to have a mixed
> mode there that will be completely different from what a mixed mode with
> the standard VESA DPCD brightness could be?
> 
> I.e. what should be the entry points for the hooks, and who calls what?

I think i915 is actually exactly where we want the hooks for this particular
backlight interface, mostly because amdgpu already had to implement their own
backlight control interface for similar reasons to Intel. From what I've seen,
the interfaces which a panel supports backlight control through tend to be
tightly tied to the hardware those panels usually accompany (at least in the
x86 world, no idea about elsewhere). For example, I've only seen laptops which
have no support/broken support for the VESA backlight interface, but not
Intel's, on hardware which only had an Intel GPU present. Every single laptop
I've tested with a Intel/Nvidia GPU setup (where the Nvidia GPU could drive the
eDP display, no idea what the situation is like elsewhere) seems to support both
interfaces perfectly. So, I don't think we'll ever see any need for this outside
of i915 at least.

Also in specific regards to the pwm control: I'm actually planning on keeping
that out of the DRM core libraries, because some variants of the VESA interface
actually need to be able to call down to PWM driver functions as well. Thus, I
think the only way of coming up with helpers for this that make sense is to only
add helpers for the DPCD related portions of backlight control that are going to
apply to all drivers. So far the only spot where we ask the driver for PWM
related info is during eDP backlight probing, where we can use it to calculate
the number of supported backlight levels, but this is entirely optional for the
driver to support it.

JFYI, here's a WIP of that:

https://gitlab.freedesktop.org/lyudess/linux/-/commit/a4bbe0d5ad980c12eb776e59a1bd522d74d09006

> 
> BR,
> Jani.
> 
> > 
> > 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.
> > 
> > Dave.
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 

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

Re: [Intel-gfx] [RFC v2 3/8] drm/i915: Keep track of pwm-related backlight hooks separately

[Lyude Paul]

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

[RFC v2 4/8] drm/i915/dp: Rename eDP VESA backlight interface functions

[Lyude Paul]

Since we're about to add support for a second type of backlight control
interface over DP AUX (specifically, Intel's proprietary HDR backlight
controls) let's rename all of the current backlight hooks we have for
vesa to make it clear that they're specific to the VESA interface and
not Intel's.

Signed-off-by: Lyude Paul <lyude@redhat.com>
Cc: thaytan@noraisin.net
Cc: Vasily Khoruzhick <anarsoul@gmail.com>
---
 .../drm/i915/display/intel_dp_aux_backlight.c | 51 ++++++++++---------
 1 file changed, 26 insertions(+), 25 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
index acbd7eb66cbe3..f601bcbe8ee46 100644
--- a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
+++ b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
@@ -25,7 +25,7 @@
 #include "intel_display_types.h"
 #include "intel_dp_aux_backlight.h"
 
-static void set_aux_backlight_enable(struct intel_dp *intel_dp, bool enable)
+static void set_vesa_backlight_enable(struct intel_dp *intel_dp, bool enable)
 {
 	struct drm_i915_private *i915 = dp_to_i915(intel_dp);
 	u8 reg_val = 0;
@@ -56,7 +56,7 @@ static void set_aux_backlight_enable(struct intel_dp *intel_dp, bool enable)
  * Read the current backlight value from DPCD register(s) based
  * on if 8-bit(MSB) or 16-bit(MSB and LSB) values are supported
  */
-static u32 intel_dp_aux_get_backlight(struct intel_connector *connector)
+static u32 intel_dp_aux_vesa_get_backlight(struct intel_connector *connector)
 {
 	struct intel_dp *intel_dp = intel_attached_dp(connector);
 	struct drm_i915_private *i915 = dp_to_i915(intel_dp);
@@ -99,7 +99,8 @@ static u32 intel_dp_aux_get_backlight(struct intel_connector *connector)
  * 8-bit or 16 bit value (MSB and LSB)
  */
 static void
-intel_dp_aux_set_backlight(const struct drm_connector_state *conn_state, u32 level)
+intel_dp_aux_vesa_set_backlight(const struct drm_connector_state *conn_state,
+				u32 level)
 {
 	struct intel_connector *connector = to_intel_connector(conn_state->connector);
 	struct intel_dp *intel_dp = intel_attached_dp(connector);
@@ -129,7 +130,7 @@ intel_dp_aux_set_backlight(const struct drm_connector_state *conn_state, u32 lev
  * - Where P = 2^Pn, where Pn is the value programmed by field 4:0 of the
  *             EDP_PWMGEN_BIT_COUNT register (DPCD Address 00724h)
  */
-static bool intel_dp_aux_set_pwm_freq(struct intel_connector *connector)
+static bool intel_dp_aux_vesa_set_pwm_freq(struct intel_connector *connector)
 {
 	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
 	struct intel_dp *intel_dp = intel_attached_dp(connector);
@@ -165,8 +166,8 @@ static bool intel_dp_aux_set_pwm_freq(struct intel_connector *connector)
 	return true;
 }
 
-static void intel_dp_aux_enable_backlight(const struct intel_crtc_state *crtc_state,
-					  const struct drm_connector_state *conn_state)
+static void intel_dp_aux_vesa_enable_backlight(const struct intel_crtc_state *crtc_state,
+					       const struct drm_connector_state *conn_state)
 {
 	struct intel_connector *connector = to_intel_connector(conn_state->connector);
 	struct intel_dp *intel_dp = intel_attached_dp(connector);
@@ -206,7 +207,7 @@ static void intel_dp_aux_enable_backlight(const struct intel_crtc_state *crtc_st
 	}
 
 	if (intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_FREQ_AUX_SET_CAP)
-		if (intel_dp_aux_set_pwm_freq(connector))
+		if (intel_dp_aux_vesa_set_pwm_freq(connector))
 			new_dpcd_buf |= DP_EDP_BACKLIGHT_FREQ_AUX_SET_ENABLE;
 
 	if (new_dpcd_buf != dpcd_buf) {
@@ -217,18 +218,18 @@ static void intel_dp_aux_enable_backlight(const struct intel_crtc_state *crtc_st
 		}
 	}
 
-	intel_dp_aux_set_backlight(conn_state,
-				   connector->panel.backlight.level);
-	set_aux_backlight_enable(intel_dp, true);
+	intel_dp_aux_vesa_set_backlight(conn_state,
+					connector->panel.backlight.level);
+	set_vesa_backlight_enable(intel_dp, true);
 }
 
-static void intel_dp_aux_disable_backlight(const struct drm_connector_state *old_conn_state)
+static void intel_dp_aux_vesa_disable_backlight(const struct drm_connector_state *old_conn_state)
 {
-	set_aux_backlight_enable(enc_to_intel_dp(to_intel_encoder(old_conn_state->best_encoder)),
-				 false);
+	set_vesa_backlight_enable(enc_to_intel_dp(to_intel_encoder(old_conn_state->best_encoder)),
+				  false);
 }
 
-static u32 intel_dp_aux_calc_max_backlight(struct intel_connector *connector)
+static u32 intel_dp_aux_vesa_calc_max_backlight(struct intel_connector *connector)
 {
 	struct drm_i915_private *i915 = to_i915(connector->base.dev);
 	struct intel_dp *intel_dp = intel_attached_dp(connector);
@@ -308,24 +309,24 @@ static u32 intel_dp_aux_calc_max_backlight(struct intel_connector *connector)
 	return max_backlight;
 }
 
-static int intel_dp_aux_setup_backlight(struct intel_connector *connector,
-					enum pipe pipe)
+static int intel_dp_aux_vesa_setup_backlight(struct intel_connector *connector,
+					     enum pipe pipe)
 {
 	struct intel_panel *panel = &connector->panel;
 
-	panel->backlight.max = intel_dp_aux_calc_max_backlight(connector);
+	panel->backlight.max = intel_dp_aux_vesa_calc_max_backlight(connector);
 	if (!panel->backlight.max)
 		return -ENODEV;
 
 	panel->backlight.min = 0;
-	panel->backlight.level = intel_dp_aux_get_backlight(connector);
+	panel->backlight.level = intel_dp_aux_vesa_get_backlight(connector);
 	panel->backlight.enabled = panel->backlight.level != 0;
 
 	return 0;
 }
 
 static bool
-intel_dp_aux_display_control_capable(struct intel_connector *connector)
+intel_dp_aux_supports_vesa_backlight(struct intel_connector *connector)
 {
 	struct intel_dp *intel_dp = intel_attached_dp(connector);
 	struct drm_i915_private *i915 = dp_to_i915(intel_dp);
@@ -349,7 +350,7 @@ int intel_dp_aux_init_backlight_funcs(struct intel_connector *intel_connector)
 	struct drm_i915_private *i915 = dp_to_i915(intel_dp);
 
 	if (i915->params.enable_dpcd_backlight == 0 ||
-	    !intel_dp_aux_display_control_capable(intel_connector))
+	    !intel_dp_aux_supports_vesa_backlight(intel_connector))
 		return -ENODEV;
 
 	/*
@@ -371,11 +372,11 @@ int intel_dp_aux_init_backlight_funcs(struct intel_connector *intel_connector)
 		return -ENODEV;
 	}
 
-	panel->backlight.setup = intel_dp_aux_setup_backlight;
-	panel->backlight.enable = intel_dp_aux_enable_backlight;
-	panel->backlight.disable = intel_dp_aux_disable_backlight;
-	panel->backlight.set = intel_dp_aux_set_backlight;
-	panel->backlight.get = intel_dp_aux_get_backlight;
+	panel->backlight.setup = intel_dp_aux_vesa_setup_backlight;
+	panel->backlight.enable = intel_dp_aux_vesa_enable_backlight;
+	panel->backlight.disable = intel_dp_aux_vesa_disable_backlight;
+	panel->backlight.set = intel_dp_aux_vesa_set_backlight;
+	panel->backlight.get = intel_dp_aux_vesa_get_backlight;
 
 	return 0;
 }
-- 
2.26.2

Re: [Intel-gfx] [RFC v2 4/8] drm/i915/dp: Rename eDP VESA backlight interface functions

[Rodrigo Vivi]

On Wed, Sep 16, 2020 at 01:18:51PM -0400, Lyude Paul wrote:
> Since we're about to add support for a second type of backlight control
> interface over DP AUX (specifically, Intel's proprietary HDR backlight
> controls) let's rename all of the current backlight hooks we have for
> vesa to make it clear that they're specific to the VESA interface and
> not Intel's.
> 
> Signed-off-by: Lyude Paul <lyude@redhat.com>
> Cc: thaytan@noraisin.net
> Cc: Vasily Khoruzhick <anarsoul@gmail.com>

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

> ---
>  .../drm/i915/display/intel_dp_aux_backlight.c | 51 ++++++++++---------
>  1 file changed, 26 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
> index acbd7eb66cbe3..f601bcbe8ee46 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
> @@ -25,7 +25,7 @@
>  #include "intel_display_types.h"
>  #include "intel_dp_aux_backlight.h"
>  
> -static void set_aux_backlight_enable(struct intel_dp *intel_dp, bool enable)
> +static void set_vesa_backlight_enable(struct intel_dp *intel_dp, bool enable)
>  {
>  	struct drm_i915_private *i915 = dp_to_i915(intel_dp);
>  	u8 reg_val = 0;
> @@ -56,7 +56,7 @@ static void set_aux_backlight_enable(struct intel_dp *intel_dp, bool enable)
>   * Read the current backlight value from DPCD register(s) based
>   * on if 8-bit(MSB) or 16-bit(MSB and LSB) values are supported
>   */
> -static u32 intel_dp_aux_get_backlight(struct intel_connector *connector)
> +static u32 intel_dp_aux_vesa_get_backlight(struct intel_connector *connector)
>  {
>  	struct intel_dp *intel_dp = intel_attached_dp(connector);
>  	struct drm_i915_private *i915 = dp_to_i915(intel_dp);
> @@ -99,7 +99,8 @@ static u32 intel_dp_aux_get_backlight(struct intel_connector *connector)
>   * 8-bit or 16 bit value (MSB and LSB)
>   */
>  static void
> -intel_dp_aux_set_backlight(const struct drm_connector_state *conn_state, u32 level)
> +intel_dp_aux_vesa_set_backlight(const struct drm_connector_state *conn_state,
> +				u32 level)
>  {
>  	struct intel_connector *connector = to_intel_connector(conn_state->connector);
>  	struct intel_dp *intel_dp = intel_attached_dp(connector);
> @@ -129,7 +130,7 @@ intel_dp_aux_set_backlight(const struct drm_connector_state *conn_state, u32 lev
>   * - Where P = 2^Pn, where Pn is the value programmed by field 4:0 of the
>   *             EDP_PWMGEN_BIT_COUNT register (DPCD Address 00724h)
>   */
> -static bool intel_dp_aux_set_pwm_freq(struct intel_connector *connector)
> +static bool intel_dp_aux_vesa_set_pwm_freq(struct intel_connector *connector)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
>  	struct intel_dp *intel_dp = intel_attached_dp(connector);
> @@ -165,8 +166,8 @@ static bool intel_dp_aux_set_pwm_freq(struct intel_connector *connector)
>  	return true;
>  }
>  
> -static void intel_dp_aux_enable_backlight(const struct intel_crtc_state *crtc_state,
> -					  const struct drm_connector_state *conn_state)
> +static void intel_dp_aux_vesa_enable_backlight(const struct intel_crtc_state *crtc_state,
> +					       const struct drm_connector_state *conn_state)
>  {
>  	struct intel_connector *connector = to_intel_connector(conn_state->connector);
>  	struct intel_dp *intel_dp = intel_attached_dp(connector);
> @@ -206,7 +207,7 @@ static void intel_dp_aux_enable_backlight(const struct intel_crtc_state *crtc_st
>  	}
>  
>  	if (intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_FREQ_AUX_SET_CAP)
> -		if (intel_dp_aux_set_pwm_freq(connector))
> +		if (intel_dp_aux_vesa_set_pwm_freq(connector))
>  			new_dpcd_buf |= DP_EDP_BACKLIGHT_FREQ_AUX_SET_ENABLE;
>  
>  	if (new_dpcd_buf != dpcd_buf) {
> @@ -217,18 +218,18 @@ static void intel_dp_aux_enable_backlight(const struct intel_crtc_state *crtc_st
>  		}
>  	}
>  
> -	intel_dp_aux_set_backlight(conn_state,
> -				   connector->panel.backlight.level);
> -	set_aux_backlight_enable(intel_dp, true);
> +	intel_dp_aux_vesa_set_backlight(conn_state,
> +					connector->panel.backlight.level);
> +	set_vesa_backlight_enable(intel_dp, true);
>  }
>  
> -static void intel_dp_aux_disable_backlight(const struct drm_connector_state *old_conn_state)
> +static void intel_dp_aux_vesa_disable_backlight(const struct drm_connector_state *old_conn_state)
>  {
> -	set_aux_backlight_enable(enc_to_intel_dp(to_intel_encoder(old_conn_state->best_encoder)),
> -				 false);
> +	set_vesa_backlight_enable(enc_to_intel_dp(to_intel_encoder(old_conn_state->best_encoder)),
> +				  false);
>  }
>  
> -static u32 intel_dp_aux_calc_max_backlight(struct intel_connector *connector)
> +static u32 intel_dp_aux_vesa_calc_max_backlight(struct intel_connector *connector)
>  {
>  	struct drm_i915_private *i915 = to_i915(connector->base.dev);
>  	struct intel_dp *intel_dp = intel_attached_dp(connector);
> @@ -308,24 +309,24 @@ static u32 intel_dp_aux_calc_max_backlight(struct intel_connector *connector)
>  	return max_backlight;
>  }
>  
> -static int intel_dp_aux_setup_backlight(struct intel_connector *connector,
> -					enum pipe pipe)
> +static int intel_dp_aux_vesa_setup_backlight(struct intel_connector *connector,
> +					     enum pipe pipe)
>  {
>  	struct intel_panel *panel = &connector->panel;
>  
> -	panel->backlight.max = intel_dp_aux_calc_max_backlight(connector);
> +	panel->backlight.max = intel_dp_aux_vesa_calc_max_backlight(connector);
>  	if (!panel->backlight.max)
>  		return -ENODEV;
>  
>  	panel->backlight.min = 0;
> -	panel->backlight.level = intel_dp_aux_get_backlight(connector);
> +	panel->backlight.level = intel_dp_aux_vesa_get_backlight(connector);
>  	panel->backlight.enabled = panel->backlight.level != 0;
>  
>  	return 0;
>  }
>  
>  static bool
> -intel_dp_aux_display_control_capable(struct intel_connector *connector)
> +intel_dp_aux_supports_vesa_backlight(struct intel_connector *connector)
>  {
>  	struct intel_dp *intel_dp = intel_attached_dp(connector);
>  	struct drm_i915_private *i915 = dp_to_i915(intel_dp);
> @@ -349,7 +350,7 @@ int intel_dp_aux_init_backlight_funcs(struct intel_connector *intel_connector)
>  	struct drm_i915_private *i915 = dp_to_i915(intel_dp);
>  
>  	if (i915->params.enable_dpcd_backlight == 0 ||
> -	    !intel_dp_aux_display_control_capable(intel_connector))
> +	    !intel_dp_aux_supports_vesa_backlight(intel_connector))
>  		return -ENODEV;
>  
>  	/*
> @@ -371,11 +372,11 @@ int intel_dp_aux_init_backlight_funcs(struct intel_connector *intel_connector)
>  		return -ENODEV;
>  	}
>  
> -	panel->backlight.setup = intel_dp_aux_setup_backlight;
> -	panel->backlight.enable = intel_dp_aux_enable_backlight;
> -	panel->backlight.disable = intel_dp_aux_disable_backlight;
> -	panel->backlight.set = intel_dp_aux_set_backlight;
> -	panel->backlight.get = intel_dp_aux_get_backlight;
> +	panel->backlight.setup = intel_dp_aux_vesa_setup_backlight;
> +	panel->backlight.enable = intel_dp_aux_vesa_enable_backlight;
> +	panel->backlight.disable = intel_dp_aux_vesa_disable_backlight;
> +	panel->backlight.set = intel_dp_aux_vesa_set_backlight;
> +	panel->backlight.get = intel_dp_aux_vesa_get_backlight;
>  
>  	return 0;
>  }
> -- 
> 2.26.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[RFC v2 5/8] drm/i915/dp: Add register definitions for Intel HDR backlight interface

[Lyude Paul]

No functional changes yet, this just adds definitions for all of the
known DPCD registers used by Intel's HDR backlight interface. Since
we'll only ever use this in i915, we just define them in
intel_dp_aux_backlight.c

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
Signed-off-by: Lyude Paul <lyude@redhat.com>
Cc: thaytan@noraisin.net
Cc: Vasily Khoruzhick <anarsoul@gmail.com>
---
 .../drm/i915/display/intel_dp_aux_backlight.c | 53 +++++++++++++++++++
 1 file changed, 53 insertions(+)

diff --git a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
index f601bcbe8ee46..c1e8e8b166267 100644
--- a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
+++ b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
@@ -25,6 +25,59 @@
 #include "intel_display_types.h"
 #include "intel_dp_aux_backlight.h"
 
+/*
+ * DP AUX registers for Intel's proprietary HDR backlight interface. We define
+ * them here since we'll likely be the only driver to ever use these.
+ */
+#define INTEL_EDP_HDR_TCON_CAP0                                        0x340
+
+#define INTEL_EDP_HDR_TCON_CAP1                                        0x341
+# define INTEL_EDP_HDR_TCON_2084_DECODE_CAP                           BIT(0)
+# define INTEL_EDP_HDR_TCON_2020_GAMUT_CAP                            BIT(1)
+# define INTEL_EDP_HDR_TCON_TONE_MAPPING_CAP                          BIT(2)
+# define INTEL_EDP_HDR_TCON_SEGMENTED_BACKLIGHT_CAP                   BIT(3)
+# define INTEL_EDP_HDR_TCON_BRIGHTNESS_NITS_CAP                       BIT(4)
+# define INTEL_EDP_HDR_TCON_OPTIMIZATION_CAP                          BIT(5)
+# define INTEL_EDP_HDR_TCON_SDP_COLORIMETRY_CAP                       BIT(6)
+# define INTEL_EDP_HDR_TCON_SRGB_TO_PANEL_GAMUT_CONVERSION_CAP        BIT(7)
+
+#define INTEL_EDP_HDR_TCON_CAP2                                        0x342
+# define INTEL_EDP_SDR_TCON_BRIGHTNESS_AUX_CAP                        BIT(0)
+
+#define INTEL_EDP_HDR_TCON_CAP3                                        0x343
+
+#define INTEL_EDP_HDR_GETSET_CTRL_PARAMS                               0x344
+# define INTEL_EDP_HDR_TCON_2084_DECODE_ENABLE                        BIT(0)
+# define INTEL_EDP_HDR_TCON_2020_GAMUT_ENABLE                         BIT(1)
+# define INTEL_EDP_HDR_TCON_TONE_MAPPING_ENABLE                       BIT(2) /* Pre-TGL+ */
+# define INTEL_EDP_HDR_TCON_SEGMENTED_BACKLIGHT_ENABLE                BIT(3)
+# define INTEL_EDP_HDR_TCON_BRIGHTNESS_AUX_ENABLE                     BIT(4)
+# define INTEL_EDP_HDR_TCON_SRGB_TO_PANEL_GAMUT_ENABLE                BIT(5)
+/* Bit 6 is reserved */
+# define INTEL_EDP_HDR_TCON_SDP_COLORIMETRY_ENABLE                    BIT(7)
+
+#define INTEL_EDP_HDR_CONTENT_LUMINANCE                                0x346 /* Pre-TGL+ */
+#define INTEL_EDP_HDR_PANEL_LUMINANCE_OVERRIDE                         0x34A
+#define INTEL_EDP_SDR_LUMINANCE_LEVEL                                  0x352
+#define INTEL_EDP_BRIGHTNESS_NITS_LSB                                  0x354
+#define INTEL_EDP_BRIGHTNESS_NITS_MSB                                  0x355
+#define INTEL_EDP_BRIGHTNESS_DELAY_FRAMES                              0x356
+#define INTEL_EDP_BRIGHTNESS_PER_FRAME_STEPS                           0x357
+
+#define INTEL_EDP_BRIGHTNESS_OPTIMIZATION_0                            0x358
+# define INTEL_EDP_TCON_USAGE_MASK                             GENMASK(0, 3)
+# define INTEL_EDP_TCON_USAGE_UNKNOWN                                    0x0
+# define INTEL_EDP_TCON_USAGE_DESKTOP                                    0x1
+# define INTEL_EDP_TCON_USAGE_FULL_SCREEN_MEDIA                          0x2
+# define INTEL_EDP_TCON_USAGE_FULL_SCREEN_GAMING                         0x3
+# define INTEL_EDP_TCON_POWER_MASK                                    BIT(4)
+# define INTEL_EDP_TCON_POWER_DC                                    (0 << 4)
+# define INTEL_EDP_TCON_POWER_AC                                    (1 << 4)
+# define INTEL_EDP_TCON_OPTIMIZATION_STRENGTH_MASK             GENMASK(5, 7)
+
+#define INTEL_EDP_BRIGHTNESS_OPTIMIZATION_1                            0x359
+
+/* VESA backlight callbacks */
 static void set_vesa_backlight_enable(struct intel_dp *intel_dp, bool enable)
 {
 	struct drm_i915_private *i915 = dp_to_i915(intel_dp);
-- 
2.26.2

[RFC v2 6/8] drm/i915/dp: Enable Intel's HDR backlight interface (only SDR for now)

[Lyude Paul]

So-recently a bunch of laptops on the market have started using DPCD
backlight controls instead of the traditional DDI backlight controls.
Originally we thought we had this handled by adding VESA backlight
control support to i915, but the story ended up being a lot more
complicated then that.

Simply put-there's two main backlight interfaces Intel can see in the
wild. Intel's proprietary HDR backlight interface, and the standard VESA
backlight interface. Note that many panels have been observed to report
support for both backlight interfaces, but testing has shown far more
panels work with the Intel HDR backlight interface at the moment.
Additionally, the VBT appears to be capable of reporting support for the
VESA backlight interface but not the Intel HDR interface which needs to
be probed by setting the right magic OUI.

On top of that however, there's also actually two different variants of
the Intel HDR backlight interface. The first uses the AUX channel for
controlling the brightness of the screen in both SDR and HDR mode, and
the second only uses the AUX channel for setting the brightness level in
HDR mode - relying on PWM for setting the brightness level in SDR mode.

For the time being we've been using EDIDs to maintain a list of quirks
for panels that safely do support the VESA backlight interface. Adding
support for Intel's HDR backlight interface in addition however, should
finally allow us to auto-detect eDP backlight controls properly so long
as we probe like so:

* If the panel's VBT reports VESA backlight support, assume it really
  does support it
* If the panel's VBT reports DDI backlight controls:
  * First probe for Intel's HDR backlight interface
  * If that fails, probe for VESA's backlight interface
  * If that fails, assume no DPCD backlight control
* If the panel's VBT reports any other backlight type: just assume it
  doesn't have DPCD backlight controls

Signed-off-by: Lyude Paul <lyude@redhat.com>
Cc: thaytan@noraisin.net
Cc: Vasily Khoruzhick <anarsoul@gmail.com>
---
 .../drm/i915/display/intel_display_types.h    |   9 +-
 .../drm/i915/display/intel_dp_aux_backlight.c | 253 ++++++++++++++++--
 drivers/gpu/drm/i915/display/intel_panel.c    |  34 ++-
 drivers/gpu/drm/i915/display/intel_panel.h    |   4 +
 4 files changed, 268 insertions(+), 32 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
index 52a6543df842a..9d540368bac89 100644
--- a/drivers/gpu/drm/i915/display/intel_display_types.h
+++ b/drivers/gpu/drm/i915/display/intel_display_types.h
@@ -230,7 +230,14 @@ struct intel_panel {
 		struct pwm_state pwm_state;
 
 		/* DPCD backlight */
-		u8 pwmgen_bit_count;
+		union {
+			struct {
+				u8 pwmgen_bit_count;
+			} vesa;
+			struct {
+				bool sdr_uses_aux;
+			} intel;
+		} edp;
 
 		struct {
 			int (*setup)(struct intel_connector *connector, enum pipe pipe);
diff --git a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
index c1e8e8b166267..376419ea3ae52 100644
--- a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
+++ b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
@@ -22,8 +22,26 @@
  *
  */
 
+/*
+ * Laptops with Intel GPUs which have panels that support controlling the
+ * backlight through DP AUX can actually use two different interfaces: Intel's
+ * proprietary DP AUX backlight interface, and the standard VESA backlight
+ * interface. Unfortunately, at the time of writing this a lot of laptops will
+ * advertise support for the standard VESA backlight interface when they
+ * don't properly support it. However, on these systems the Intel backlight
+ * interface generally does work properly. Additionally, these systems will
+ * usually just indicate that they use PWM backlight controls in their VBIOS
+ * for some reason.
+ */
+
 #include "intel_display_types.h"
 #include "intel_dp_aux_backlight.h"
+#include "intel_panel.h"
+
+/* TODO:
+ * Implement HDR, right now we just implement the bare minimum to bring us back into SDR mode so we
+ * can make people's backlights work in the mean time
+ */
 
 /*
  * DP AUX registers for Intel's proprietary HDR backlight interface. We define
@@ -77,6 +95,176 @@
 
 #define INTEL_EDP_BRIGHTNESS_OPTIMIZATION_1                            0x359
 
+/* Intel EDP backlight callbacks */
+static bool
+intel_dp_aux_supports_hdr_backlight(struct intel_connector *connector)
+{
+	struct drm_device *dev = connector->base.dev;
+	struct intel_dp *intel_dp = enc_to_intel_dp(connector->encoder);
+	struct drm_dp_aux *aux = &intel_dp->aux;
+	struct intel_panel *panel = &connector->panel;
+	int ret;
+	u8 tcon_cap[4];
+
+	ret = drm_dp_dpcd_read(aux, INTEL_EDP_HDR_TCON_CAP0, tcon_cap, sizeof(tcon_cap));
+	if (ret < 0)
+		return false;
+
+	if (!(tcon_cap[1] & INTEL_EDP_HDR_TCON_BRIGHTNESS_NITS_CAP))
+		return false;
+
+	if (tcon_cap[0] >= 1) {
+		drm_dbg_kms(dev, "Detected Intel HDR backlight interface version %d\n",
+			    tcon_cap[0]);
+	} else {
+		drm_dbg_kms(dev, "Detected unsupported HDR backlight interface version %d\n",
+			    tcon_cap[0]);
+		return false;
+	}
+
+	panel->backlight.edp.intel.sdr_uses_aux =
+		tcon_cap[2] & INTEL_EDP_SDR_TCON_BRIGHTNESS_AUX_CAP;
+
+	return true;
+}
+
+static u32
+intel_dp_aux_hdr_get_backlight(struct intel_connector *connector)
+{
+	struct drm_device *dev = connector->base.dev;
+	struct intel_panel *panel = &connector->panel;
+	struct intel_dp *intel_dp = enc_to_intel_dp(connector->encoder);
+	u8 tmp;
+	u8 buf[2] = { 0 };
+
+	if (drm_dp_dpcd_readb(&intel_dp->aux, INTEL_EDP_HDR_GETSET_CTRL_PARAMS, &tmp) < 0)
+		drm_err(dev, "Failed to read current backlight mode from DPCD\n");
+
+	if (!(tmp & INTEL_EDP_HDR_TCON_BRIGHTNESS_AUX_ENABLE)) {
+		if (panel->backlight.edp.intel.sdr_uses_aux) {
+			/* Assume 100% brightness if backlight controls aren't enabled yet */
+			return panel->backlight.max;
+		} else {
+			u32 pwm_level = panel->backlight.pwm_funcs.get(connector);
+
+			return intel_panel_backlight_level_from_pwm(connector, pwm_level);
+		}
+	}
+
+	if (drm_dp_dpcd_read(&intel_dp->aux, INTEL_EDP_BRIGHTNESS_NITS_LSB, buf, sizeof(buf)) < 0) {
+		drm_err(dev, "Failed to read brightness from DPCD\n");
+		return 0;
+	}
+
+	return (buf[1] << 8 | buf[0]);
+}
+
+static void
+intel_dp_aux_hdr_set_aux_backlight(const struct drm_connector_state *conn_state, u32 level)
+{
+	struct intel_connector *connector = to_intel_connector(conn_state->connector);
+	struct drm_device *dev = connector->base.dev;
+	struct intel_dp *intel_dp = enc_to_intel_dp(connector->encoder);
+	uint8_t buf[4] = { 0 };
+
+	buf[0] = level & 0xFF;
+	buf[1] = (level & 0xFF00) >> 8;
+
+	if (drm_dp_dpcd_write(&intel_dp->aux, INTEL_EDP_BRIGHTNESS_NITS_LSB, buf, 4) < 0)
+		drm_err(dev, "Failed to write brightness level to DPCD\n");
+}
+
+static void
+intel_dp_aux_hdr_set_backlight(const struct drm_connector_state *conn_state, u32 level)
+{
+	struct intel_connector *connector = to_intel_connector(conn_state->connector);
+	struct intel_panel *panel = &connector->panel;
+
+	if (panel->backlight.edp.intel.sdr_uses_aux) {
+		intel_dp_aux_hdr_set_aux_backlight(conn_state, level);
+	} else {
+		const u32 pwm_level = intel_panel_backlight_level_to_pwm(connector, level);
+		intel_panel_set_pwm_level(conn_state, pwm_level);
+	}
+}
+
+static void
+intel_dp_aux_hdr_enable_backlight(const struct intel_crtc_state *crtc_state,
+				  const struct drm_connector_state *conn_state)
+{
+	struct intel_connector *connector = to_intel_connector(conn_state->connector);
+	struct intel_panel *panel = &connector->panel;
+	struct drm_device *dev = connector->base.dev;
+	struct intel_dp *intel_dp = enc_to_intel_dp(connector->encoder);
+	int ret;
+	u8 old_ctrl, ctrl;
+
+	ret = drm_dp_dpcd_readb(&intel_dp->aux, INTEL_EDP_HDR_GETSET_CTRL_PARAMS, &old_ctrl);
+	if (ret < 0) {
+		drm_err(dev, "Failed to read current backlight control mode: %d\n", ret);
+		return;
+	}
+
+	ctrl = old_ctrl;
+	if (panel->backlight.edp.intel.sdr_uses_aux) {
+		ctrl |= INTEL_EDP_HDR_TCON_BRIGHTNESS_AUX_ENABLE;
+		intel_dp_aux_hdr_set_aux_backlight(conn_state, panel->backlight.level);
+	} else {
+		u32 pwm_level = intel_panel_backlight_level_to_pwm(connector,
+								   panel->backlight.level);
+		panel->backlight.pwm_funcs.enable(crtc_state, conn_state, pwm_level);
+
+		ctrl &= ~(INTEL_EDP_HDR_TCON_BRIGHTNESS_AUX_ENABLE);
+	}
+
+	if (ctrl != old_ctrl)
+		if (drm_dp_dpcd_writeb(&intel_dp->aux, INTEL_EDP_HDR_GETSET_CTRL_PARAMS, ctrl) < 0)
+			drm_err(dev, "Failed to configure DPCD brightness controls\n");
+}
+
+static void
+intel_dp_aux_hdr_disable_backlight(const struct drm_connector_state *conn_state)
+{
+	struct intel_connector *connector = to_intel_connector(conn_state->connector);
+	struct intel_panel *panel = &connector->panel;
+
+	/* Nothing to do for AUX based backlight controls */
+	if (panel->backlight.edp.intel.sdr_uses_aux)
+		return;
+
+	/* Note we want the actual pwm_level to be 0, regardless of pwm_min */
+	panel->backlight.pwm_funcs.disable(conn_state,
+					   intel_panel_sanitize_pwm_level(connector, 0));
+}
+
+static int
+intel_dp_aux_hdr_setup_backlight(struct intel_connector *connector, enum pipe pipe)
+{
+	struct drm_device *dev = connector->base.dev;
+	struct intel_panel *panel = &connector->panel;
+	int ret;
+
+	if (panel->backlight.edp.intel.sdr_uses_aux) {
+		drm_dbg_kms(dev, "SDR backlight is controlled through DPCD\n");
+	} else {
+		drm_dbg_kms(dev, "SDR backlight is controlled through PWM\n");
+
+		ret = panel->backlight.pwm_funcs.setup(connector, pipe);
+		if (ret < 0) {
+			drm_err(dev, "Failed to setup SDR backlight controls through PWM: %d\n",
+				ret);
+			return ret;
+		}
+	}
+
+	panel->backlight.max = 512;
+	panel->backlight.min = 0;
+	panel->backlight.level = intel_dp_aux_hdr_get_backlight(connector);
+	panel->backlight.enabled = panel->backlight.level != 0;
+
+	return 0;
+}
+
 /* VESA backlight callbacks */
 static void set_vesa_backlight_enable(struct intel_dp *intel_dp, bool enable)
 {
@@ -187,7 +375,7 @@ static bool intel_dp_aux_vesa_set_pwm_freq(struct intel_connector *connector)
 {
 	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
 	struct intel_dp *intel_dp = intel_attached_dp(connector);
-	const u8 pn = connector->panel.backlight.pwmgen_bit_count;
+	const u8 pn = connector->panel.backlight.edp.vesa.pwmgen_bit_count;
 	int freq, fxp, f, fxp_actual, fxp_min, fxp_max;
 
 	freq = dev_priv->vbt.backlight.pwm_freq_hz;
@@ -227,6 +415,7 @@ static void intel_dp_aux_vesa_enable_backlight(const struct intel_crtc_state *cr
 	struct drm_i915_private *i915 = dp_to_i915(intel_dp);
 	struct intel_panel *panel = &connector->panel;
 	u8 dpcd_buf, new_dpcd_buf, edp_backlight_mode;
+	u8 pwmgen_bit_count = panel->backlight.edp.vesa.pwmgen_bit_count;
 
 	if (drm_dp_dpcd_readb(&intel_dp->aux,
 			DP_EDP_BACKLIGHT_MODE_SET_REGISTER, &dpcd_buf) != 1) {
@@ -247,7 +436,7 @@ static void intel_dp_aux_vesa_enable_backlight(const struct intel_crtc_state *cr
 
 		if (drm_dp_dpcd_writeb(&intel_dp->aux,
 				       DP_EDP_PWMGEN_BIT_COUNT,
-				       panel->backlight.pwmgen_bit_count) < 0)
+				       pwmgen_bit_count) < 0)
 			drm_dbg_kms(&i915->drm,
 				    "Failed to write aux pwmgen bit count\n");
 
@@ -355,7 +544,7 @@ static u32 intel_dp_aux_vesa_calc_max_backlight(struct intel_connector *connecto
 			    "Failed to write aux pwmgen bit count\n");
 		return max_backlight;
 	}
-	panel->backlight.pwmgen_bit_count = pn;
+	panel->backlight.edp.vesa.pwmgen_bit_count = pn;
 
 	max_backlight = (1 << pn) - 1;
 
@@ -396,40 +585,46 @@ intel_dp_aux_supports_vesa_backlight(struct intel_connector *connector)
 	return false;
 }
 
-int intel_dp_aux_init_backlight_funcs(struct intel_connector *intel_connector)
+int intel_dp_aux_init_backlight_funcs(struct intel_connector *connector)
 {
-	struct intel_panel *panel = &intel_connector->panel;
-	struct intel_dp *intel_dp = enc_to_intel_dp(intel_connector->encoder);
+	struct drm_device *dev = connector->base.dev;
+	struct intel_panel *panel = &connector->panel;
+	struct intel_dp *intel_dp = enc_to_intel_dp(connector->encoder);
 	struct drm_i915_private *i915 = dp_to_i915(intel_dp);
 
-	if (i915->params.enable_dpcd_backlight == 0 ||
-	    !intel_dp_aux_supports_vesa_backlight(intel_connector))
+	if (i915->params.enable_dpcd_backlight == 0)
 		return -ENODEV;
 
 	/*
-	 * There are a lot of machines that don't advertise the backlight
-	 * control interface to use properly in their VBIOS, :\
+	 * A lot of eDP panels in the wild will report supporting both the
+	 * Intel proprietary backlight control interface, and the VESA
+	 * backlight control interface. Many of these panels are liars though,
+	 * and will only work with the Intel interface. So, always probe for
+	 * that first.
 	 */
-	if (i915->vbt.backlight.type !=
-	    INTEL_BACKLIGHT_VESA_EDP_AUX_INTERFACE &&
-	    i915->params.enable_dpcd_backlight != 1 &&
-	    !drm_dp_has_quirk(&intel_dp->desc, intel_dp->edid_quirks,
-			      DP_QUIRK_FORCE_DPCD_BACKLIGHT)) {
-		drm_info(&i915->drm,
-			 "Panel advertises DPCD backlight support, but "
-			 "VBT disagrees. If your backlight controls "
-			 "don't work try booting with "
-			 "i915.enable_dpcd_backlight=1. If your machine "
-			 "needs this, please file a _new_ bug report on "
-			 "drm/i915, see " FDO_BUG_URL " for details.\n");
-		return -ENODEV;
+	if (intel_dp_aux_supports_hdr_backlight(connector)) {
+		drm_dbg(dev, "Using Intel proprietary eDP backlight controls\n");
+
+		panel->backlight.setup = intel_dp_aux_hdr_setup_backlight;
+		panel->backlight.enable = intel_dp_aux_hdr_enable_backlight;
+		panel->backlight.disable = intel_dp_aux_hdr_disable_backlight;
+		panel->backlight.set = intel_dp_aux_hdr_set_backlight;
+		panel->backlight.get = intel_dp_aux_hdr_get_backlight;
+
+		return 0;
 	}
 
-	panel->backlight.setup = intel_dp_aux_vesa_setup_backlight;
-	panel->backlight.enable = intel_dp_aux_vesa_enable_backlight;
-	panel->backlight.disable = intel_dp_aux_vesa_disable_backlight;
-	panel->backlight.set = intel_dp_aux_vesa_set_backlight;
-	panel->backlight.get = intel_dp_aux_vesa_get_backlight;
+	if (intel_dp_aux_supports_vesa_backlight(connector)) {
+		drm_dbg(dev, "Using VESA eDP backlight controls\n");
 
-	return 0;
+		panel->backlight.setup = intel_dp_aux_vesa_setup_backlight;
+		panel->backlight.enable = intel_dp_aux_vesa_enable_backlight;
+		panel->backlight.disable = intel_dp_aux_vesa_disable_backlight;
+		panel->backlight.set = intel_dp_aux_vesa_set_backlight;
+		panel->backlight.get = intel_dp_aux_vesa_get_backlight;
+
+		return 0;
+	}
+
+	return -ENODEV;
 }
diff --git a/drivers/gpu/drm/i915/display/intel_panel.c b/drivers/gpu/drm/i915/display/intel_panel.c
index 6d3e9d51d069c..75aca9f2ffeb2 100644
--- a/drivers/gpu/drm/i915/display/intel_panel.c
+++ b/drivers/gpu/drm/i915/display/intel_panel.c
@@ -511,7 +511,7 @@ static u32 scale_hw_to_user(struct intel_connector *connector,
 		     0, user_max);
 }
 
-static u32 intel_panel_sanitize_pwm_level(struct intel_connector *connector, u32 val)
+u32 intel_panel_sanitize_pwm_level(struct intel_connector *connector, u32 val)
 {
 	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
 	struct intel_panel *panel = &connector->panel;
@@ -529,7 +529,7 @@ static u32 intel_panel_sanitize_pwm_level(struct intel_connector *connector, u32
 	return val;
 }
 
-static void intel_panel_set_pwm_level(const struct drm_connector_state *conn_state, u32 val)
+void intel_panel_set_pwm_level(const struct drm_connector_state *conn_state, u32 val)
 {
 	struct intel_connector *connector = to_intel_connector(conn_state->connector);
 	struct drm_i915_private *i915 = to_i915(connector->base.dev);
@@ -539,6 +539,36 @@ static void intel_panel_set_pwm_level(const struct drm_connector_state *conn_sta
 	panel->backlight.pwm_funcs.set(conn_state, val);
 }
 
+u32 intel_panel_backlight_level_to_pwm(struct intel_connector *connector, u32 val)
+{
+	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
+	struct intel_panel *panel = &connector->panel;
+
+	drm_WARN_ON_ONCE(&dev_priv->drm,
+			 panel->backlight.max == 0 || panel->backlight.pwm_max == 0);
+
+	val = scale(val, panel->backlight.min, panel->backlight.max,
+		    panel->backlight.pwm_min, panel->backlight.pwm_max);
+
+	return intel_panel_sanitize_pwm_level(connector, val);
+}
+
+u32 intel_panel_backlight_level_from_pwm(struct intel_connector *connector, u32 val)
+{
+	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
+	struct intel_panel *panel = &connector->panel;
+
+	drm_WARN_ON_ONCE(&dev_priv->drm,
+			 panel->backlight.max == 0 || panel->backlight.pwm_max == 0);
+
+	if (dev_priv->params.invert_brightness > 0 ||
+	    (dev_priv->params.invert_brightness == 0 && dev_priv->quirks & QUIRK_INVERT_BRIGHTNESS))
+		val = panel->backlight.pwm_max - (val - panel->backlight.pwm_min);
+
+	return scale(val, panel->backlight.pwm_min, panel->backlight.pwm_max,
+		     panel->backlight.min, panel->backlight.max);
+}
+
 static u32 lpt_get_backlight(struct intel_connector *connector)
 {
 	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
diff --git a/drivers/gpu/drm/i915/display/intel_panel.h b/drivers/gpu/drm/i915/display/intel_panel.h
index 5b813fe90557c..a548347a975f5 100644
--- a/drivers/gpu/drm/i915/display/intel_panel.h
+++ b/drivers/gpu/drm/i915/display/intel_panel.h
@@ -49,6 +49,10 @@ struct drm_display_mode *
 intel_panel_edid_fixed_mode(struct intel_connector *connector);
 struct drm_display_mode *
 intel_panel_vbt_fixed_mode(struct intel_connector *connector);
+void intel_panel_set_pwm_level(const struct drm_connector_state *conn_state, u32 level);
+u32 intel_panel_sanitize_pwm_level(struct intel_connector *connector, u32 level);
+u32 intel_panel_backlight_level_to_pwm(struct intel_connector *connector, u32 level);
+u32 intel_panel_backlight_level_from_pwm(struct intel_connector *connector, u32 val);
 
 #if IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE)
 int intel_backlight_device_register(struct intel_connector *connector);
-- 
2.26.2

Re: [RFC v2 6/8] drm/i915/dp: Enable Intel's HDR backlight interface (only SDR for now)

[Jani Nikula]

On Wed, 16 Sep 2020, Lyude Paul <lyude@redhat.com> wrote:
> So-recently a bunch of laptops on the market have started using DPCD
> backlight controls instead of the traditional DDI backlight controls.
> Originally we thought we had this handled by adding VESA backlight
> control support to i915, but the story ended up being a lot more
> complicated then that.
>
> Simply put-there's two main backlight interfaces Intel can see in the
> wild. Intel's proprietary HDR backlight interface, and the standard VESA
> backlight interface. Note that many panels have been observed to report
> support for both backlight interfaces, but testing has shown far more
> panels work with the Intel HDR backlight interface at the moment.
> Additionally, the VBT appears to be capable of reporting support for the
> VESA backlight interface but not the Intel HDR interface which needs to
> be probed by setting the right magic OUI.
>
> On top of that however, there's also actually two different variants of
> the Intel HDR backlight interface. The first uses the AUX channel for
> controlling the brightness of the screen in both SDR and HDR mode, and
> the second only uses the AUX channel for setting the brightness level in
> HDR mode - relying on PWM for setting the brightness level in SDR mode.
>
> For the time being we've been using EDIDs to maintain a list of quirks
> for panels that safely do support the VESA backlight interface. Adding
> support for Intel's HDR backlight interface in addition however, should
> finally allow us to auto-detect eDP backlight controls properly so long
> as we probe like so:
>
> * If the panel's VBT reports VESA backlight support, assume it really
>   does support it
> * If the panel's VBT reports DDI backlight controls:
>   * First probe for Intel's HDR backlight interface
>   * If that fails, probe for VESA's backlight interface
>   * If that fails, assume no DPCD backlight control
> * If the panel's VBT reports any other backlight type: just assume it
>   doesn't have DPCD backlight controls
>
> Signed-off-by: Lyude Paul <lyude@redhat.com>
> Cc: thaytan@noraisin.net
> Cc: Vasily Khoruzhick <anarsoul@gmail.com>
> ---
>  .../drm/i915/display/intel_display_types.h    |   9 +-
>  .../drm/i915/display/intel_dp_aux_backlight.c | 253 ++++++++++++++++--
>  drivers/gpu/drm/i915/display/intel_panel.c    |  34 ++-
>  drivers/gpu/drm/i915/display/intel_panel.h    |   4 +
>  4 files changed, 268 insertions(+), 32 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
> index 52a6543df842a..9d540368bac89 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> @@ -230,7 +230,14 @@ struct intel_panel {
>  		struct pwm_state pwm_state;
>  
>  		/* DPCD backlight */
> -		u8 pwmgen_bit_count;
> +		union {
> +			struct {
> +				u8 pwmgen_bit_count;
> +			} vesa;
> +			struct {
> +				bool sdr_uses_aux;
> +			} intel;
> +		} edp;
>  
>  		struct {
>  			int (*setup)(struct intel_connector *connector, enum pipe pipe);
> diff --git a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
> index c1e8e8b166267..376419ea3ae52 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
> @@ -22,8 +22,26 @@
>   *
>   */
>  
> +/*
> + * Laptops with Intel GPUs which have panels that support controlling the
> + * backlight through DP AUX can actually use two different interfaces: Intel's
> + * proprietary DP AUX backlight interface, and the standard VESA backlight
> + * interface. Unfortunately, at the time of writing this a lot of laptops will
> + * advertise support for the standard VESA backlight interface when they
> + * don't properly support it. However, on these systems the Intel backlight
> + * interface generally does work properly. Additionally, these systems will
> + * usually just indicate that they use PWM backlight controls in their VBIOS
> + * for some reason.
> + */
> +
>  #include "intel_display_types.h"
>  #include "intel_dp_aux_backlight.h"
> +#include "intel_panel.h"
> +
> +/* TODO:
> + * Implement HDR, right now we just implement the bare minimum to bring us back into SDR mode so we
> + * can make people's backlights work in the mean time
> + */
>  
>  /*
>   * DP AUX registers for Intel's proprietary HDR backlight interface. We define
> @@ -77,6 +95,176 @@
>  
>  #define INTEL_EDP_BRIGHTNESS_OPTIMIZATION_1                            0x359
>  
> +/* Intel EDP backlight callbacks */
> +static bool
> +intel_dp_aux_supports_hdr_backlight(struct intel_connector *connector)
> +{
> +	struct drm_device *dev = connector->base.dev;
> +	struct intel_dp *intel_dp = enc_to_intel_dp(connector->encoder);
> +	struct drm_dp_aux *aux = &intel_dp->aux;
> +	struct intel_panel *panel = &connector->panel;
> +	int ret;
> +	u8 tcon_cap[4];
> +
> +	ret = drm_dp_dpcd_read(aux, INTEL_EDP_HDR_TCON_CAP0, tcon_cap, sizeof(tcon_cap));
> +	if (ret < 0)
> +		return false;
> +
> +	if (!(tcon_cap[1] & INTEL_EDP_HDR_TCON_BRIGHTNESS_NITS_CAP))
> +		return false;
> +
> +	if (tcon_cap[0] >= 1) {
> +		drm_dbg_kms(dev, "Detected Intel HDR backlight interface version %d\n",
> +			    tcon_cap[0]);
> +	} else {
> +		drm_dbg_kms(dev, "Detected unsupported HDR backlight interface version %d\n",
> +			    tcon_cap[0]);
> +		return false;
> +	}
> +
> +	panel->backlight.edp.intel.sdr_uses_aux =
> +		tcon_cap[2] & INTEL_EDP_SDR_TCON_BRIGHTNESS_AUX_CAP;
> +
> +	return true;
> +}
> +
> +static u32
> +intel_dp_aux_hdr_get_backlight(struct intel_connector *connector)
> +{
> +	struct drm_device *dev = connector->base.dev;
> +	struct intel_panel *panel = &connector->panel;
> +	struct intel_dp *intel_dp = enc_to_intel_dp(connector->encoder);
> +	u8 tmp;
> +	u8 buf[2] = { 0 };
> +
> +	if (drm_dp_dpcd_readb(&intel_dp->aux, INTEL_EDP_HDR_GETSET_CTRL_PARAMS, &tmp) < 0)
> +		drm_err(dev, "Failed to read current backlight mode from DPCD\n");
> +
> +	if (!(tmp & INTEL_EDP_HDR_TCON_BRIGHTNESS_AUX_ENABLE)) {
> +		if (panel->backlight.edp.intel.sdr_uses_aux) {
> +			/* Assume 100% brightness if backlight controls aren't enabled yet */
> +			return panel->backlight.max;
> +		} else {
> +			u32 pwm_level = panel->backlight.pwm_funcs.get(connector);
> +
> +			return intel_panel_backlight_level_from_pwm(connector, pwm_level);
> +		}
> +	}
> +
> +	if (drm_dp_dpcd_read(&intel_dp->aux, INTEL_EDP_BRIGHTNESS_NITS_LSB, buf, sizeof(buf)) < 0) {
> +		drm_err(dev, "Failed to read brightness from DPCD\n");
> +		return 0;
> +	}
> +
> +	return (buf[1] << 8 | buf[0]);
> +}
> +
> +static void
> +intel_dp_aux_hdr_set_aux_backlight(const struct drm_connector_state *conn_state, u32 level)
> +{
> +	struct intel_connector *connector = to_intel_connector(conn_state->connector);
> +	struct drm_device *dev = connector->base.dev;
> +	struct intel_dp *intel_dp = enc_to_intel_dp(connector->encoder);
> +	uint8_t buf[4] = { 0 };
> +
> +	buf[0] = level & 0xFF;
> +	buf[1] = (level & 0xFF00) >> 8;
> +
> +	if (drm_dp_dpcd_write(&intel_dp->aux, INTEL_EDP_BRIGHTNESS_NITS_LSB, buf, 4) < 0)
> +		drm_err(dev, "Failed to write brightness level to DPCD\n");
> +}
> +
> +static void
> +intel_dp_aux_hdr_set_backlight(const struct drm_connector_state *conn_state, u32 level)
> +{
> +	struct intel_connector *connector = to_intel_connector(conn_state->connector);
> +	struct intel_panel *panel = &connector->panel;
> +
> +	if (panel->backlight.edp.intel.sdr_uses_aux) {
> +		intel_dp_aux_hdr_set_aux_backlight(conn_state, level);
> +	} else {
> +		const u32 pwm_level = intel_panel_backlight_level_to_pwm(connector, level);
> +		intel_panel_set_pwm_level(conn_state, pwm_level);
> +	}
> +}
> +
> +static void
> +intel_dp_aux_hdr_enable_backlight(const struct intel_crtc_state *crtc_state,
> +				  const struct drm_connector_state *conn_state)
> +{
> +	struct intel_connector *connector = to_intel_connector(conn_state->connector);
> +	struct intel_panel *panel = &connector->panel;
> +	struct drm_device *dev = connector->base.dev;
> +	struct intel_dp *intel_dp = enc_to_intel_dp(connector->encoder);
> +	int ret;
> +	u8 old_ctrl, ctrl;
> +
> +	ret = drm_dp_dpcd_readb(&intel_dp->aux, INTEL_EDP_HDR_GETSET_CTRL_PARAMS, &old_ctrl);
> +	if (ret < 0) {
> +		drm_err(dev, "Failed to read current backlight control mode: %d\n", ret);
> +		return;
> +	}
> +
> +	ctrl = old_ctrl;
> +	if (panel->backlight.edp.intel.sdr_uses_aux) {
> +		ctrl |= INTEL_EDP_HDR_TCON_BRIGHTNESS_AUX_ENABLE;
> +		intel_dp_aux_hdr_set_aux_backlight(conn_state, panel->backlight.level);
> +	} else {
> +		u32 pwm_level = intel_panel_backlight_level_to_pwm(connector,
> +								   panel->backlight.level);
> +		panel->backlight.pwm_funcs.enable(crtc_state, conn_state, pwm_level);
> +
> +		ctrl &= ~(INTEL_EDP_HDR_TCON_BRIGHTNESS_AUX_ENABLE);

This is what I was referring to in my reply to another patch, regarding
who calls what.

We can have a number of combinations with the standard VESA interface,
the proprietary interface and its modes, the eDP PWM pin control, and
the mixed modes.

In particular the mixed modes require the use of both. If we move the
VESA code to drm core as helpers, they can't have any i915 specific in
them.

So here's what I'm thinking. For eDP brightness control, could we
organize this under a higher level API, perhaps in the existing API in
intel_panel.c, that calls the DPCD brightness hooks (either VESA or
proprietary), *if* available, and according to return values from those
hooks, optionally *also* calls the eDP PWM hooks for mixed mode?

In pseudo code:

	bool call_pwm = true;

	if (backlight->aux_funcs->hook)
        	call_pwm = backlight->aux_funcs->hook();

	if (call_pwm)
        	backlight->pwm_funcs->hook();

Could we define the aux_funcs in a way that the interface could be
generic enough to have drm aux helpers assigned there? i.e. with the
current VESA code moved to core. Or, HDR hooks as the case may be.

I stress that I'm not requesting you do all of this now. But I really
would like to have a view into how we could make this work neatly in the
future, with a) standard VESA DPCD brightness code moved to drm core,
and b) the standard VESA + eDP PWM pin mixed mode working, c) the
proprietary DPCD + eDP PWM pin mixed mode working, in a way that aren't
wildly different from each other?

BR,
Jani.


> +	}
> +
> +	if (ctrl != old_ctrl)
> +		if (drm_dp_dpcd_writeb(&intel_dp->aux, INTEL_EDP_HDR_GETSET_CTRL_PARAMS, ctrl) < 0)
> +			drm_err(dev, "Failed to configure DPCD brightness controls\n");
> +}
> +
> +static void
> +intel_dp_aux_hdr_disable_backlight(const struct drm_connector_state *conn_state)
> +{
> +	struct intel_connector *connector = to_intel_connector(conn_state->connector);
> +	struct intel_panel *panel = &connector->panel;
> +
> +	/* Nothing to do for AUX based backlight controls */
> +	if (panel->backlight.edp.intel.sdr_uses_aux)
> +		return;
> +
> +	/* Note we want the actual pwm_level to be 0, regardless of pwm_min */
> +	panel->backlight.pwm_funcs.disable(conn_state,
> +					   intel_panel_sanitize_pwm_level(connector, 0));
> +}
> +
> +static int
> +intel_dp_aux_hdr_setup_backlight(struct intel_connector *connector, enum pipe pipe)
> +{
> +	struct drm_device *dev = connector->base.dev;
> +	struct intel_panel *panel = &connector->panel;
> +	int ret;
> +
> +	if (panel->backlight.edp.intel.sdr_uses_aux) {
> +		drm_dbg_kms(dev, "SDR backlight is controlled through DPCD\n");
> +	} else {
> +		drm_dbg_kms(dev, "SDR backlight is controlled through PWM\n");
> +
> +		ret = panel->backlight.pwm_funcs.setup(connector, pipe);
> +		if (ret < 0) {
> +			drm_err(dev, "Failed to setup SDR backlight controls through PWM: %d\n",
> +				ret);
> +			return ret;
> +		}
> +	}
> +
> +	panel->backlight.max = 512;
> +	panel->backlight.min = 0;
> +	panel->backlight.level = intel_dp_aux_hdr_get_backlight(connector);
> +	panel->backlight.enabled = panel->backlight.level != 0;
> +
> +	return 0;
> +}
> +
>  /* VESA backlight callbacks */
>  static void set_vesa_backlight_enable(struct intel_dp *intel_dp, bool enable)
>  {
> @@ -187,7 +375,7 @@ static bool intel_dp_aux_vesa_set_pwm_freq(struct intel_connector *connector)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
>  	struct intel_dp *intel_dp = intel_attached_dp(connector);
> -	const u8 pn = connector->panel.backlight.pwmgen_bit_count;
> +	const u8 pn = connector->panel.backlight.edp.vesa.pwmgen_bit_count;
>  	int freq, fxp, f, fxp_actual, fxp_min, fxp_max;
>  
>  	freq = dev_priv->vbt.backlight.pwm_freq_hz;
> @@ -227,6 +415,7 @@ static void intel_dp_aux_vesa_enable_backlight(const struct intel_crtc_state *cr
>  	struct drm_i915_private *i915 = dp_to_i915(intel_dp);
>  	struct intel_panel *panel = &connector->panel;
>  	u8 dpcd_buf, new_dpcd_buf, edp_backlight_mode;
> +	u8 pwmgen_bit_count = panel->backlight.edp.vesa.pwmgen_bit_count;
>  
>  	if (drm_dp_dpcd_readb(&intel_dp->aux,
>  			DP_EDP_BACKLIGHT_MODE_SET_REGISTER, &dpcd_buf) != 1) {
> @@ -247,7 +436,7 @@ static void intel_dp_aux_vesa_enable_backlight(const struct intel_crtc_state *cr
>  
>  		if (drm_dp_dpcd_writeb(&intel_dp->aux,
>  				       DP_EDP_PWMGEN_BIT_COUNT,
> -				       panel->backlight.pwmgen_bit_count) < 0)
> +				       pwmgen_bit_count) < 0)
>  			drm_dbg_kms(&i915->drm,
>  				    "Failed to write aux pwmgen bit count\n");
>  
> @@ -355,7 +544,7 @@ static u32 intel_dp_aux_vesa_calc_max_backlight(struct intel_connector *connecto
>  			    "Failed to write aux pwmgen bit count\n");
>  		return max_backlight;
>  	}
> -	panel->backlight.pwmgen_bit_count = pn;
> +	panel->backlight.edp.vesa.pwmgen_bit_count = pn;
>  
>  	max_backlight = (1 << pn) - 1;
>  
> @@ -396,40 +585,46 @@ intel_dp_aux_supports_vesa_backlight(struct intel_connector *connector)
>  	return false;
>  }
>  
> -int intel_dp_aux_init_backlight_funcs(struct intel_connector *intel_connector)
> +int intel_dp_aux_init_backlight_funcs(struct intel_connector *connector)
>  {
> -	struct intel_panel *panel = &intel_connector->panel;
> -	struct intel_dp *intel_dp = enc_to_intel_dp(intel_connector->encoder);
> +	struct drm_device *dev = connector->base.dev;
> +	struct intel_panel *panel = &connector->panel;
> +	struct intel_dp *intel_dp = enc_to_intel_dp(connector->encoder);
>  	struct drm_i915_private *i915 = dp_to_i915(intel_dp);
>  
> -	if (i915->params.enable_dpcd_backlight == 0 ||
> -	    !intel_dp_aux_supports_vesa_backlight(intel_connector))
> +	if (i915->params.enable_dpcd_backlight == 0)
>  		return -ENODEV;
>  
>  	/*
> -	 * There are a lot of machines that don't advertise the backlight
> -	 * control interface to use properly in their VBIOS, :\
> +	 * A lot of eDP panels in the wild will report supporting both the
> +	 * Intel proprietary backlight control interface, and the VESA
> +	 * backlight control interface. Many of these panels are liars though,
> +	 * and will only work with the Intel interface. So, always probe for
> +	 * that first.
>  	 */
> -	if (i915->vbt.backlight.type !=
> -	    INTEL_BACKLIGHT_VESA_EDP_AUX_INTERFACE &&
> -	    i915->params.enable_dpcd_backlight != 1 &&
> -	    !drm_dp_has_quirk(&intel_dp->desc, intel_dp->edid_quirks,
> -			      DP_QUIRK_FORCE_DPCD_BACKLIGHT)) {
> -		drm_info(&i915->drm,
> -			 "Panel advertises DPCD backlight support, but "
> -			 "VBT disagrees. If your backlight controls "
> -			 "don't work try booting with "
> -			 "i915.enable_dpcd_backlight=1. If your machine "
> -			 "needs this, please file a _new_ bug report on "
> -			 "drm/i915, see " FDO_BUG_URL " for details.\n");
> -		return -ENODEV;
> +	if (intel_dp_aux_supports_hdr_backlight(connector)) {
> +		drm_dbg(dev, "Using Intel proprietary eDP backlight controls\n");
> +
> +		panel->backlight.setup = intel_dp_aux_hdr_setup_backlight;
> +		panel->backlight.enable = intel_dp_aux_hdr_enable_backlight;
> +		panel->backlight.disable = intel_dp_aux_hdr_disable_backlight;
> +		panel->backlight.set = intel_dp_aux_hdr_set_backlight;
> +		panel->backlight.get = intel_dp_aux_hdr_get_backlight;
> +
> +		return 0;
>  	}
>  
> -	panel->backlight.setup = intel_dp_aux_vesa_setup_backlight;
> -	panel->backlight.enable = intel_dp_aux_vesa_enable_backlight;
> -	panel->backlight.disable = intel_dp_aux_vesa_disable_backlight;
> -	panel->backlight.set = intel_dp_aux_vesa_set_backlight;
> -	panel->backlight.get = intel_dp_aux_vesa_get_backlight;
> +	if (intel_dp_aux_supports_vesa_backlight(connector)) {
> +		drm_dbg(dev, "Using VESA eDP backlight controls\n");
>  
> -	return 0;
> +		panel->backlight.setup = intel_dp_aux_vesa_setup_backlight;
> +		panel->backlight.enable = intel_dp_aux_vesa_enable_backlight;
> +		panel->backlight.disable = intel_dp_aux_vesa_disable_backlight;
> +		panel->backlight.set = intel_dp_aux_vesa_set_backlight;
> +		panel->backlight.get = intel_dp_aux_vesa_get_backlight;
> +
> +		return 0;
> +	}
> +
> +	return -ENODEV;
>  }
> diff --git a/drivers/gpu/drm/i915/display/intel_panel.c b/drivers/gpu/drm/i915/display/intel_panel.c
> index 6d3e9d51d069c..75aca9f2ffeb2 100644
> --- a/drivers/gpu/drm/i915/display/intel_panel.c
> +++ b/drivers/gpu/drm/i915/display/intel_panel.c
> @@ -511,7 +511,7 @@ static u32 scale_hw_to_user(struct intel_connector *connector,
>  		     0, user_max);
>  }
>  
> -static u32 intel_panel_sanitize_pwm_level(struct intel_connector *connector, u32 val)
> +u32 intel_panel_sanitize_pwm_level(struct intel_connector *connector, u32 val)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
>  	struct intel_panel *panel = &connector->panel;
> @@ -529,7 +529,7 @@ static u32 intel_panel_sanitize_pwm_level(struct intel_connector *connector, u32
>  	return val;
>  }
>  
> -static void intel_panel_set_pwm_level(const struct drm_connector_state *conn_state, u32 val)
> +void intel_panel_set_pwm_level(const struct drm_connector_state *conn_state, u32 val)
>  {
>  	struct intel_connector *connector = to_intel_connector(conn_state->connector);
>  	struct drm_i915_private *i915 = to_i915(connector->base.dev);
> @@ -539,6 +539,36 @@ static void intel_panel_set_pwm_level(const struct drm_connector_state *conn_sta
>  	panel->backlight.pwm_funcs.set(conn_state, val);
>  }
>  
> +u32 intel_panel_backlight_level_to_pwm(struct intel_connector *connector, u32 val)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
> +	struct intel_panel *panel = &connector->panel;
> +
> +	drm_WARN_ON_ONCE(&dev_priv->drm,
> +			 panel->backlight.max == 0 || panel->backlight.pwm_max == 0);
> +
> +	val = scale(val, panel->backlight.min, panel->backlight.max,
> +		    panel->backlight.pwm_min, panel->backlight.pwm_max);
> +
> +	return intel_panel_sanitize_pwm_level(connector, val);
> +}
> +
> +u32 intel_panel_backlight_level_from_pwm(struct intel_connector *connector, u32 val)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
> +	struct intel_panel *panel = &connector->panel;
> +
> +	drm_WARN_ON_ONCE(&dev_priv->drm,
> +			 panel->backlight.max == 0 || panel->backlight.pwm_max == 0);
> +
> +	if (dev_priv->params.invert_brightness > 0 ||
> +	    (dev_priv->params.invert_brightness == 0 && dev_priv->quirks & QUIRK_INVERT_BRIGHTNESS))
> +		val = panel->backlight.pwm_max - (val - panel->backlight.pwm_min);
> +
> +	return scale(val, panel->backlight.pwm_min, panel->backlight.pwm_max,
> +		     panel->backlight.min, panel->backlight.max);
> +}
> +
>  static u32 lpt_get_backlight(struct intel_connector *connector)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
> diff --git a/drivers/gpu/drm/i915/display/intel_panel.h b/drivers/gpu/drm/i915/display/intel_panel.h
> index 5b813fe90557c..a548347a975f5 100644
> --- a/drivers/gpu/drm/i915/display/intel_panel.h
> +++ b/drivers/gpu/drm/i915/display/intel_panel.h
> @@ -49,6 +49,10 @@ struct drm_display_mode *
>  intel_panel_edid_fixed_mode(struct intel_connector *connector);
>  struct drm_display_mode *
>  intel_panel_vbt_fixed_mode(struct intel_connector *connector);
> +void intel_panel_set_pwm_level(const struct drm_connector_state *conn_state, u32 level);
> +u32 intel_panel_sanitize_pwm_level(struct intel_connector *connector, u32 level);
> +u32 intel_panel_backlight_level_to_pwm(struct intel_connector *connector, u32 level);
> +u32 intel_panel_backlight_level_from_pwm(struct intel_connector *connector, u32 val);
>  
>  #if IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE)
>  int intel_backlight_device_register(struct intel_connector *connector);

-- 
Jani Nikula, Intel Open Source Graphics Center

[RFC v2 7/8] drm/i915/dp: Allow forcing specific interfaces through enable_dpcd_backlight

[Lyude Paul]

Since we now support controlling panel backlights through DPCD using
both the standard VESA interface, and Intel's proprietary HDR backlight
interface, we should allow the user to be able to explicitly choose
between one or the other in the event that we're wrong about panels
reliably reporting support for the Intel HDR interface.

So, this commit adds support for this by introducing two new
enable_dpcd_backlight options: 2 which forces i915 to only probe for the
VESA interface, and 3 which forces i915 to only probe for the Intel
backlight interface (might be useful if we find panels in the wild that
report the VESA interface in their VBT, but actually only support the
Intel backlight interface).

Signed-off-by: Lyude Paul <lyude@redhat.com>
Cc: thaytan@noraisin.net
Cc: Vasily Khoruzhick <anarsoul@gmail.com>
---
 .../drm/i915/display/intel_dp_aux_backlight.c | 45 +++++++++++++++++--
 drivers/gpu/drm/i915/i915_params.c            |  2 +-
 2 files changed, 43 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
index 376419ea3ae52..aa1429302db70 100644
--- a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
+++ b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
@@ -585,15 +585,54 @@ intel_dp_aux_supports_vesa_backlight(struct intel_connector *connector)
 	return false;
 }
 
+enum intel_dp_aux_backlight_modparam {
+	INTEL_DP_AUX_BACKLIGHT_AUTO = -1,
+	INTEL_DP_AUX_BACKLIGHT_OFF = 0,
+	INTEL_DP_AUX_BACKLIGHT_ON = 1,
+	INTEL_DP_AUX_BACKLIGHT_FORCE_VESA = 2,
+	INTEL_DP_AUX_BACKLIGHT_FORCE_INTEL = 3,
+};
+
 int intel_dp_aux_init_backlight_funcs(struct intel_connector *connector)
 {
 	struct drm_device *dev = connector->base.dev;
 	struct intel_panel *panel = &connector->panel;
 	struct intel_dp *intel_dp = enc_to_intel_dp(connector->encoder);
 	struct drm_i915_private *i915 = dp_to_i915(intel_dp);
+	bool try_intel_interface = false, try_vesa_interface = false;
 
-	if (i915->params.enable_dpcd_backlight == 0)
+	/* Check the VBT and user's module parameters to figure out which
+	 * interfaces to probe
+	 */
+	switch (i915->params.enable_dpcd_backlight) {
+	case INTEL_DP_AUX_BACKLIGHT_OFF:
 		return -ENODEV;
+	case INTEL_DP_AUX_BACKLIGHT_AUTO:
+		switch (i915->vbt.backlight.type) {
+		case INTEL_BACKLIGHT_VESA_EDP_AUX_INTERFACE:
+			try_vesa_interface = true;
+			break;
+		case INTEL_BACKLIGHT_DISPLAY_DDI:
+			try_intel_interface = true;
+			try_vesa_interface = true;
+			break;
+		default:
+			return -ENODEV;
+		}
+		break;
+	case INTEL_DP_AUX_BACKLIGHT_ON:
+		if (i915->vbt.backlight.type != INTEL_BACKLIGHT_VESA_EDP_AUX_INTERFACE)
+			try_intel_interface = true;
+
+		try_vesa_interface = true;
+		break;
+	case INTEL_DP_AUX_BACKLIGHT_FORCE_VESA:
+		try_vesa_interface = true;
+		break;
+	case INTEL_DP_AUX_BACKLIGHT_FORCE_INTEL:
+		try_intel_interface = true;
+		break;
+	}
 
 	/*
 	 * A lot of eDP panels in the wild will report supporting both the
@@ -602,7 +641,7 @@ int intel_dp_aux_init_backlight_funcs(struct intel_connector *connector)
 	 * and will only work with the Intel interface. So, always probe for
 	 * that first.
 	 */
-	if (intel_dp_aux_supports_hdr_backlight(connector)) {
+	if (try_intel_interface && intel_dp_aux_supports_hdr_backlight(connector)) {
 		drm_dbg(dev, "Using Intel proprietary eDP backlight controls\n");
 
 		panel->backlight.setup = intel_dp_aux_hdr_setup_backlight;
@@ -614,7 +653,7 @@ int intel_dp_aux_init_backlight_funcs(struct intel_connector *connector)
 		return 0;
 	}
 
-	if (intel_dp_aux_supports_vesa_backlight(connector)) {
+	if (try_vesa_interface && intel_dp_aux_supports_vesa_backlight(connector)) {
 		drm_dbg(dev, "Using VESA eDP backlight controls\n");
 
 		panel->backlight.setup = intel_dp_aux_vesa_setup_backlight;
diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
index 7f139ea4a90b2..6939634e56ed6 100644
--- a/drivers/gpu/drm/i915/i915_params.c
+++ b/drivers/gpu/drm/i915/i915_params.c
@@ -185,7 +185,7 @@ i915_param_named_unsafe(inject_probe_failure, uint, 0400,
 
 i915_param_named(enable_dpcd_backlight, int, 0400,
 	"Enable support for DPCD backlight control"
-	"(-1=use per-VBT LFP backlight type setting [default], 0=disabled, 1=enabled)");
+	"(-1=use per-VBT LFP backlight type setting [default], 0=disabled, 1=enable, 2=force VESA interface, 3=force Intel interface)");
 
 #if IS_ENABLED(CONFIG_DRM_I915_GVT)
 i915_param_named(enable_gvt, bool, 0400,
-- 
2.26.2

[RFC v2 8/8] drm/dp: Revert "drm/dp: Introduce EDID-based quirks"

[Lyude Paul]

This reverts commit 0883ce8146ed6074c76399f4e70dbed788582e12. Originally
these quirks were added because of the issues with using the eDP
backlight interfaces on certain laptop panels, which made it impossible
to properly probe for DPCD backlight support without having a whitelist
for panels that we know have working VESA backlight control interfaces
over DPCD. As well, it should be noted it was impossible to use the
normal sink OUI for recognizing these panels as none of them actually
filled out their OUIs, hence needing to resort to checking EDIDs.

At the time we weren't really sure why certain panels had issues with
DPCD backlight controls, but we eventually figured out that there was a
second interface that these problematic laptop panels actually did work
with and advertise properly: Intel's proprietary backlight interface for
HDR panels. So far the testing we've done hasn't brought any panels to
light that advertise this interface and don't support it properly, which
means we finally have a real solution to this problem.

As a result, we now have no need for the force DPCD backlight quirk, and
furthermore this also removes the need for any kind of EDID quirk
checking in DRM. So, let's just revert it for now since we were the only
driver using this.

v2:
* Fix indenting error picked up by checkpatch in
  intel_edp_init_connector()

Signed-off-by: Lyude Paul <lyude@redhat.com>
Acked-by: Jani Nikula <jani.nikula@intel.com>
Cc: thaytan@noraisin.net
Cc: Vasily Khoruzhick <anarsoul@gmail.com>
---
 drivers/gpu/drm/drm_dp_helper.c               | 82 +------------------
 drivers/gpu/drm/drm_dp_mst_topology.c         |  3 +-
 .../drm/i915/display/intel_display_types.h    |  1 -
 drivers/gpu/drm/i915/display/intel_dp.c       |  9 +-
 drivers/gpu/drm/i915/display/intel_dp_mst.c   |  3 +-
 drivers/gpu/drm/i915/display/intel_psr.c      |  2 +-
 include/drm/drm_dp_helper.h                   | 21 +----
 7 files changed, 9 insertions(+), 112 deletions(-)

diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
index 1e7c638873c82..7138655bfc9d0 100644
--- a/drivers/gpu/drm/drm_dp_helper.c
+++ b/drivers/gpu/drm/drm_dp_helper.c
@@ -823,7 +823,7 @@ bool drm_dp_read_sink_count_cap(struct drm_connector *connector,
 	return connector->connector_type != DRM_MODE_CONNECTOR_eDP &&
 		dpcd[DP_DPCD_REV] >= DP_DPCD_REV_11 &&
 		dpcd[DP_DOWNSTREAMPORT_PRESENT] & DP_DWN_STRM_PORT_PRESENT &&
-		!drm_dp_has_quirk(desc, 0, DP_DPCD_QUIRK_NO_SINK_COUNT);
+		!drm_dp_has_quirk(desc, DP_DPCD_QUIRK_NO_SINK_COUNT);
 }
 EXPORT_SYMBOL(drm_dp_read_sink_count_cap);
 
@@ -1544,86 +1544,6 @@ drm_dp_get_quirks(const struct drm_dp_dpcd_ident *ident, bool is_branch)
 #undef DEVICE_ID_ANY
 #undef DEVICE_ID
 
-struct edid_quirk {
-	u8 mfg_id[2];
-	u8 prod_id[2];
-	u32 quirks;
-};
-
-#define MFG(first, second) { (first), (second) }
-#define PROD_ID(first, second) { (first), (second) }
-
-/*
- * Some devices have unreliable OUIDs where they don't set the device ID
- * correctly, and as a result we need to use the EDID for finding additional
- * DP quirks in such cases.
- */
-static const struct edid_quirk edid_quirk_list[] = {
-	/* Optional 4K AMOLED panel in the ThinkPad X1 Extreme 2nd Generation
-	 * only supports DPCD backlight controls
-	 */
-	{ MFG(0x4c, 0x83), PROD_ID(0x41, 0x41), BIT(DP_QUIRK_FORCE_DPCD_BACKLIGHT) },
-	/*
-	 * Some Dell CML 2020 systems have panels support both AUX and PWM
-	 * backlight control, and some only support AUX backlight control. All
-	 * said panels start up in AUX mode by default, and we don't have any
-	 * support for disabling HDR mode on these panels which would be
-	 * required to switch to PWM backlight control mode (plus, I'm not
-	 * even sure we want PWM backlight controls over DPCD backlight
-	 * controls anyway...). Until we have a better way of detecting these,
-	 * force DPCD backlight mode on all of them.
-	 */
-	{ MFG(0x06, 0xaf), PROD_ID(0x9b, 0x32), BIT(DP_QUIRK_FORCE_DPCD_BACKLIGHT) },
-	{ MFG(0x06, 0xaf), PROD_ID(0xeb, 0x41), BIT(DP_QUIRK_FORCE_DPCD_BACKLIGHT) },
-	{ MFG(0x4d, 0x10), PROD_ID(0xc7, 0x14), BIT(DP_QUIRK_FORCE_DPCD_BACKLIGHT) },
-	{ MFG(0x4d, 0x10), PROD_ID(0xe6, 0x14), BIT(DP_QUIRK_FORCE_DPCD_BACKLIGHT) },
-	{ MFG(0x4c, 0x83), PROD_ID(0x47, 0x41), BIT(DP_QUIRK_FORCE_DPCD_BACKLIGHT) },
-};
-
-#undef MFG
-#undef PROD_ID
-
-/**
- * drm_dp_get_edid_quirks() - Check the EDID of a DP device to find additional
- * DP-specific quirks
- * @edid: The EDID to check
- *
- * While OUIDs are meant to be used to recognize a DisplayPort device, a lot
- * of manufacturers don't seem to like following standards and neglect to fill
- * the dev-ID in, making it impossible to only use OUIDs for determining
- * quirks in some cases. This function can be used to check the EDID and look
- * up any additional DP quirks. The bits returned by this function correspond
- * to the quirk bits in &drm_dp_quirk.
- *
- * Returns: a bitmask of quirks, if any. The driver can check this using
- * drm_dp_has_quirk().
- */
-u32 drm_dp_get_edid_quirks(const struct edid *edid)
-{
-	const struct edid_quirk *quirk;
-	u32 quirks = 0;
-	int i;
-
-	if (!edid)
-		return 0;
-
-	for (i = 0; i < ARRAY_SIZE(edid_quirk_list); i++) {
-		quirk = &edid_quirk_list[i];
-		if (memcmp(quirk->mfg_id, edid->mfg_id,
-			   sizeof(edid->mfg_id)) == 0 &&
-		    memcmp(quirk->prod_id, edid->prod_code,
-			   sizeof(edid->prod_code)) == 0)
-			quirks |= quirk->quirks;
-	}
-
-	DRM_DEBUG_KMS("DP sink: EDID mfg %*phD prod-ID %*phD quirks: 0x%04x\n",
-		      (int)sizeof(edid->mfg_id), edid->mfg_id,
-		      (int)sizeof(edid->prod_code), edid->prod_code, quirks);
-
-	return quirks;
-}
-EXPORT_SYMBOL(drm_dp_get_edid_quirks);
-
 /**
  * drm_dp_read_desc - read sink/branch descriptor from DPCD
  * @aux: DisplayPort AUX channel
diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
index e875425336406..f21516142dd50 100644
--- a/drivers/gpu/drm/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/drm_dp_mst_topology.c
@@ -5824,8 +5824,7 @@ struct drm_dp_aux *drm_dp_mst_dsc_aux_for_port(struct drm_dp_mst_port *port)
 	if (drm_dp_read_desc(port->mgr->aux, &desc, true))
 		return NULL;
 
-	if (drm_dp_has_quirk(&desc, 0,
-			     DP_DPCD_QUIRK_DSC_WITHOUT_VIRTUAL_DPCD) &&
+	if (drm_dp_has_quirk(&desc, DP_DPCD_QUIRK_DSC_WITHOUT_VIRTUAL_DPCD) &&
 	    port->mgr->dpcd[DP_DPCD_REV] >= DP_DPCD_REV_14 &&
 	    port->parent == port->mgr->mst_primary) {
 		u8 downstreamport;
diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
index 9d540368bac89..0bf378903644c 100644
--- a/drivers/gpu/drm/i915/display/intel_display_types.h
+++ b/drivers/gpu/drm/i915/display/intel_display_types.h
@@ -1322,7 +1322,6 @@ struct intel_dp {
 	int max_link_rate;
 	/* sink or branch descriptor */
 	struct drm_dp_desc desc;
-	u32 edid_quirks;
 	struct drm_dp_aux aux;
 	u32 aux_busy_last_status;
 	u8 train_set[4];
diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
index 7db2b6a3cd52e..34f09b043580b 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -162,8 +162,7 @@ static void intel_dp_set_sink_rates(struct intel_dp *intel_dp)
 	};
 	int i, max_rate;
 
-	if (drm_dp_has_quirk(&intel_dp->desc, 0,
-			     DP_DPCD_QUIRK_CAN_DO_MAX_LINK_RATE_3_24_GBPS)) {
+	if (drm_dp_has_quirk(&intel_dp->desc, DP_DPCD_QUIRK_CAN_DO_MAX_LINK_RATE_3_24_GBPS)) {
 		/* Needed, e.g., for Apple MBP 2017, 15 inch eDP Retina panel */
 		static const int quirk_rates[] = { 162000, 270000, 324000 };
 
@@ -2630,8 +2629,7 @@ intel_dp_compute_config(struct intel_encoder *encoder,
 	struct intel_connector *intel_connector = intel_dp->attached_connector;
 	struct intel_digital_connector_state *intel_conn_state =
 		to_intel_digital_connector_state(conn_state);
-	bool constant_n = drm_dp_has_quirk(&intel_dp->desc, 0,
-					   DP_DPCD_QUIRK_CONSTANT_N);
+	bool constant_n = drm_dp_has_quirk(&intel_dp->desc, DP_DPCD_QUIRK_CONSTANT_N);
 	int ret = 0, output_bpp;
 
 	if (HAS_PCH_SPLIT(dev_priv) && !HAS_DDI(dev_priv) && port != PORT_A)
@@ -6104,7 +6102,6 @@ intel_dp_set_edid(struct intel_dp *intel_dp)
 
 	intel_dp->has_audio = drm_detect_monitor_audio(edid);
 	drm_dp_cec_set_edid(&intel_dp->aux, edid);
-	intel_dp->edid_quirks = drm_dp_get_edid_quirks(edid);
 }
 
 static void
@@ -6117,7 +6114,6 @@ intel_dp_unset_edid(struct intel_dp *intel_dp)
 	intel_connector->detect_edid = NULL;
 
 	intel_dp->has_audio = false;
-	intel_dp->edid_quirks = 0;
 }
 
 static int
@@ -7469,7 +7465,6 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
 	if (edid) {
 		if (drm_add_edid_modes(connector, edid)) {
 			drm_connector_update_edid_property(connector, edid);
-			intel_dp->edid_quirks = drm_dp_get_edid_quirks(edid);
 		} else {
 			kfree(edid);
 			edid = ERR_PTR(-EINVAL);
diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
index 64d885539e94a..c8b9ffa388cf5 100644
--- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
@@ -52,8 +52,7 @@ static int intel_dp_mst_compute_link_config(struct intel_encoder *encoder,
 	struct drm_i915_private *i915 = to_i915(connector->base.dev);
 	const struct drm_display_mode *adjusted_mode =
 		&crtc_state->hw.adjusted_mode;
-	bool constant_n = drm_dp_has_quirk(&intel_dp->desc, 0,
-					   DP_DPCD_QUIRK_CONSTANT_N);
+	bool constant_n = drm_dp_has_quirk(&intel_dp->desc, DP_DPCD_QUIRK_CONSTANT_N);
 	int bpp, slots = -EINVAL;
 
 	crtc_state->lane_count = limits->max_lane_count;
diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c
index 8a9d0bdde1bfb..c7403406e2783 100644
--- a/drivers/gpu/drm/i915/display/intel_psr.c
+++ b/drivers/gpu/drm/i915/display/intel_psr.c
@@ -310,7 +310,7 @@ void intel_psr_init_dpcd(struct intel_dp *intel_dp)
 	drm_dbg_kms(&dev_priv->drm, "eDP panel supports PSR version %x\n",
 		    intel_dp->psr_dpcd[0]);
 
-	if (drm_dp_has_quirk(&intel_dp->desc, 0, DP_DPCD_QUIRK_NO_PSR)) {
+	if (drm_dp_has_quirk(&intel_dp->desc, DP_DPCD_QUIRK_NO_PSR)) {
 		drm_dbg_kms(&dev_priv->drm,
 			    "PSR support not currently available for this panel\n");
 		return;
diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
index 5c45195ced321..09dd81007dba4 100644
--- a/include/drm/drm_dp_helper.h
+++ b/include/drm/drm_dp_helper.h
@@ -1671,16 +1671,13 @@ struct drm_dp_desc {
 
 int drm_dp_read_desc(struct drm_dp_aux *aux, struct drm_dp_desc *desc,
 		     bool is_branch);
-u32 drm_dp_get_edid_quirks(const struct edid *edid);
 
 /**
  * enum drm_dp_quirk - Display Port sink/branch device specific quirks
  *
  * Display Port sink and branch devices in the wild have a variety of bugs, try
  * to collect them here. The quirks are shared, but it's up to the drivers to
- * implement workarounds for them. Note that because some devices have
- * unreliable OUIDs, the EDID of sinks should also be checked for quirks using
- * drm_dp_get_edid_quirks().
+ * implement workarounds for them.
  */
 enum drm_dp_quirk {
 	/**
@@ -1712,16 +1709,6 @@ enum drm_dp_quirk {
 	 * The DSC caps can be read from the physical aux instead.
 	 */
 	DP_DPCD_QUIRK_DSC_WITHOUT_VIRTUAL_DPCD,
-	/**
-	 * @DP_QUIRK_FORCE_DPCD_BACKLIGHT:
-	 *
-	 * The device is telling the truth when it says that it uses DPCD
-	 * backlight controls, even if the system's firmware disagrees. This
-	 * quirk should be checked against both the ident and panel EDID.
-	 * When present, the driver should honor the DPCD backlight
-	 * capabilities advertised.
-	 */
-	DP_QUIRK_FORCE_DPCD_BACKLIGHT,
 	/**
 	 * @DP_DPCD_QUIRK_CAN_DO_MAX_LINK_RATE_3_24_GBPS:
 	 *
@@ -1734,16 +1721,14 @@ enum drm_dp_quirk {
 /**
  * drm_dp_has_quirk() - does the DP device have a specific quirk
  * @desc: Device descriptor filled by drm_dp_read_desc()
- * @edid_quirks: Optional quirk bitmask filled by drm_dp_get_edid_quirks()
  * @quirk: Quirk to query for
  *
  * Return true if DP device identified by @desc has @quirk.
  */
 static inline bool
-drm_dp_has_quirk(const struct drm_dp_desc *desc, u32 edid_quirks,
-		 enum drm_dp_quirk quirk)
+drm_dp_has_quirk(const struct drm_dp_desc *desc, enum drm_dp_quirk quirk)
 {
-	return (desc->quirks | edid_quirks) & BIT(quirk);
+	return desc->quirks & BIT(quirk);
 }
 
 #ifdef CONFIG_DRM_DP_CEC
-- 
2.26.2