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 <[email protected]>
Cc: [email protected]
Cc: Vasily Khoruzhick <[email protected]>
---
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
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 <[email protected]>
> Cc: [email protected]
> Cc: Vasily Khoruzhick <[email protected]>
> ---
> 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 <[email protected]>
> + 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
> [email protected]
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Wed, Sep 16, 2020 at 10:19 AM Lyude Paul <[email protected]> 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 <[email protected]>
> Cc: [email protected]
> Cc: Vasily Khoruzhick <[email protected]>
Hi Lyude,
Sorry for a late reply.
Whole series is:
Tested-by: Vasily Khoruzhick <[email protected]>
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
>
On Wed, 16 Sep 2020, Lyude Paul <[email protected]> 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 <[email protected]>
> Cc: [email protected]
> Cc: Vasily Khoruzhick <[email protected]>
> ---
> 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
On Thu, 2020-11-26 at 12:51 +0200, Jani Nikula wrote:
> On Wed, 16 Sep 2020, Lyude Paul <[email protected]> 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 <[email protected]>
> > Cc: [email protected]
> > Cc: Vasily Khoruzhick <[email protected]>
> > ---
> > 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