| Message ID | 20200915172939.2810538-2-lyude@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: Add support for Intel's eDP backlight controls | expand |
On Tue, Sep 15, 2020 at 01:29:35PM -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. > > 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 | 32 +++++++++++++++++++++++++ > 1 file changed, 32 insertions(+) > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c > index 4bd10456ad188..b591672ec4eab 100644 > --- a/drivers/gpu/drm/i915/display/intel_dp.c > +++ b/drivers/gpu/drm/i915/display/intel_dp.c > @@ -3428,6 +3428,7 @@ void intel_dp_sink_set_decompression_state(struct intel_dp *intel_dp, > void intel_dp_sink_dpms(struct intel_dp *intel_dp, int mode) > { > struct drm_i915_private *i915 = dp_to_i915(intel_dp); > + u8 edp_oui[] = { 0x00, 0xaa, 0x01 }; what are these values? > int ret, i; > > /* Should have a valid DPCD by this point */ > @@ -3443,6 +3444,14 @@ 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)) { > + ret = drm_dp_dpcd_write(&intel_dp->aux, DP_SOURCE_OUI, edp_oui, > + sizeof(edp_oui)); > + if (ret < 0) > + drm_err(&i915->drm, "Failed to write eDP source OUI\n"); > + } > + > /* > * When turning on, we need to retry for 1ms to give the sink > * time to wake up. > @@ -4530,6 +4539,23 @@ static void intel_dp_get_dsc_sink_cap(struct intel_dp *intel_dp) > } > } > > +static void > +intel_edp_init_source_oui(struct intel_dp *intel_dp) > +{ > + struct drm_i915_private *i915 = dp_to_i915(intel_dp); > + u8 oui[] = { 0x00, 0xaa, 0x01 }; > + u8 buf[3] = { 0 }; > + > + 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"); > +} > + > static bool > intel_edp_init_dpcd(struct intel_dp *intel_dp) > { > @@ -4607,6 +4633,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); > > + /* > + * 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); I believe we should restrict this to the supported platforms: cfl, whl, cml, icl, tgl no? > + > return true; > } > > -- > 2.26.2 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Tue, 2020-09-15 at 15:06 -0400, Rodrigo Vivi wrote: > On Tue, Sep 15, 2020 at 01:29:35PM -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. > > > > 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 | 32 +++++++++++++++++++++++++ > > 1 file changed, 32 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c > > b/drivers/gpu/drm/i915/display/intel_dp.c > > index 4bd10456ad188..b591672ec4eab 100644 > > --- a/drivers/gpu/drm/i915/display/intel_dp.c > > +++ b/drivers/gpu/drm/i915/display/intel_dp.c > > @@ -3428,6 +3428,7 @@ void intel_dp_sink_set_decompression_state(struct > > intel_dp *intel_dp, > > void intel_dp_sink_dpms(struct intel_dp *intel_dp, int mode) > > { > > struct drm_i915_private *i915 = dp_to_i915(intel_dp); > > + u8 edp_oui[] = { 0x00, 0xaa, 0x01 }; > > what are these values? I wish I knew, my assumption is this is the OUI that Intel's GPU driver uses on other platforms, but I don't have any documentation mentioning this (in fact, the few documents I do have on this backlight interface don't seem to make any mention of it). I only managed to find this when looking through the last attempt someone did at adding support for this backlight interface: https://patchwork.freedesktop.org/patch/334989/ I think it should be fairly safe to write, as I know nouveau always programs a source OUI (we don't do it from our driver, but nvidia hardware seems to do it automatically) and I don't believe I've seen it ever change any behavior besides making things appear in the 0x300-0x3ff register range. AFAICT though, the backlight interface won't advertise itself without this being set early on. If you could find anyone from Intel who knows more about it though I'd definitely appreciate it (and just in general for the rest of the patch series as well) > > > int ret, i; > > > > /* Should have a valid DPCD by this point */ > > @@ -3443,6 +3444,14 @@ 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)) { > > + ret = drm_dp_dpcd_write(&intel_dp->aux, DP_SOURCE_OUI, > > edp_oui, > > + sizeof(edp_oui)); > > + if (ret < 0) > > + drm_err(&i915->drm, "Failed to write eDP source > > OUI\n"); > > + } > > + > > /* > > * When turning on, we need to retry for 1ms to give the sink > > * time to wake up. > > @@ -4530,6 +4539,23 @@ static void intel_dp_get_dsc_sink_cap(struct intel_dp > > *intel_dp) > > } > > } > > > > +static void > > +intel_edp_init_source_oui(struct intel_dp *intel_dp) > > +{ > > + struct drm_i915_private *i915 = dp_to_i915(intel_dp); > > + u8 oui[] = { 0x00, 0xaa, 0x01 }; > > + u8 buf[3] = { 0 }; > > + > > + 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"); > > +} > > + > > static bool > > intel_edp_init_dpcd(struct intel_dp *intel_dp) > > { > > @@ -4607,6 +4633,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); > > > > + /* > > + * 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); > > I believe we should restrict this to the supported platforms: cfl, whl, cml, > icl, tgl > no? > > > + > > return true; > > } > > > > -- > > 2.26.2 > > > > _______________________________________________ > > dri-devel mailing list > > dri-devel@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Tue, Sep 15, 2020 at 03:47:01PM -0400, Lyude Paul wrote: > On Tue, 2020-09-15 at 15:06 -0400, Rodrigo Vivi wrote: > > On Tue, Sep 15, 2020 at 01:29:35PM -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. > > > > > > 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 | 32 +++++++++++++++++++++++++ > > > 1 file changed, 32 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c > > > b/drivers/gpu/drm/i915/display/intel_dp.c > > > index 4bd10456ad188..b591672ec4eab 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_dp.c > > > +++ b/drivers/gpu/drm/i915/display/intel_dp.c > > > @@ -3428,6 +3428,7 @@ void intel_dp_sink_set_decompression_state(struct > > > intel_dp *intel_dp, > > > void intel_dp_sink_dpms(struct intel_dp *intel_dp, int mode) > > > { > > > struct drm_i915_private *i915 = dp_to_i915(intel_dp); > > > + u8 edp_oui[] = { 0x00, 0xaa, 0x01 }; > > > > what are these values? We in i915 typically use the OUI number for adding any eDP specific quirks. I have seen these getting spit out in the dmesg log at thebeginning. AFAIK It is some kind of OEM identification number for a display panel. Manasi > > I wish I knew, my assumption is this is the OUI that Intel's GPU driver uses on > other platforms, but I don't have any documentation mentioning this (in fact, > the few documents I do have on this backlight interface don't seem to make any > mention of it). I only managed to find this when looking through the last > attempt someone did at adding support for this backlight interface: > > https://patchwork.freedesktop.org/patch/334989/ > > I think it should be fairly safe to write, as I know nouveau always programs a > source OUI (we don't do it from our driver, but nvidia hardware seems to do it > automatically) and I don't believe I've seen it ever change any behavior besides > making things appear in the 0x300-0x3ff register range. > > AFAICT though, the backlight interface won't advertise itself without this being > set early on. If you could find anyone from Intel who knows more about it though > I'd definitely appreciate it (and just in general for the rest of the patch > series as well) > > > > > > int ret, i; > > > > > > /* Should have a valid DPCD by this point */ > > > @@ -3443,6 +3444,14 @@ 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)) { > > > + ret = drm_dp_dpcd_write(&intel_dp->aux, DP_SOURCE_OUI, > > > edp_oui, > > > + sizeof(edp_oui)); > > > + if (ret < 0) > > > + drm_err(&i915->drm, "Failed to write eDP source > > > OUI\n"); > > > + } > > > + > > > /* > > > * When turning on, we need to retry for 1ms to give the sink > > > * time to wake up. > > > @@ -4530,6 +4539,23 @@ static void intel_dp_get_dsc_sink_cap(struct intel_dp > > > *intel_dp) > > > } > > > } > > > > > > +static void > > > +intel_edp_init_source_oui(struct intel_dp *intel_dp) > > > +{ > > > + struct drm_i915_private *i915 = dp_to_i915(intel_dp); > > > + u8 oui[] = { 0x00, 0xaa, 0x01 }; > > > + u8 buf[3] = { 0 }; > > > + > > > + 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"); > > > +} > > > + > > > static bool > > > intel_edp_init_dpcd(struct intel_dp *intel_dp) > > > { > > > @@ -4607,6 +4633,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); > > > > > > + /* > > > + * 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); > > > > I believe we should restrict this to the supported platforms: cfl, whl, cml, > > icl, tgl > > no? > > > > > + > > > return true; > > > } > > > > > > -- > > > 2.26.2 > > > > > > _______________________________________________ > > > dri-devel mailing list > > > dri-devel@lists.freedesktop.org > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel > -- > Sincerely, > Lyude Paul (she/her) > Software Engineer at Red Hat >
On Tue, 2020-09-15 at 15:38 -0700, Navare, Manasi wrote: > On Tue, Sep 15, 2020 at 03:47:01PM -0400, Lyude Paul wrote: > > On Tue, 2020-09-15 at 15:06 -0400, Rodrigo Vivi wrote: > > > On Tue, Sep 15, 2020 at 01:29:35PM -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. > > > > > > > > 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 | 32 > > > > +++++++++++++++++++++++++ > > > > 1 file changed, 32 insertions(+) > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c > > > > b/drivers/gpu/drm/i915/display/intel_dp.c > > > > index 4bd10456ad188..b591672ec4eab 100644 > > > > --- a/drivers/gpu/drm/i915/display/intel_dp.c > > > > +++ b/drivers/gpu/drm/i915/display/intel_dp.c > > > > @@ -3428,6 +3428,7 @@ void > > > > intel_dp_sink_set_decompression_state(struct > > > > intel_dp *intel_dp, > > > > void intel_dp_sink_dpms(struct intel_dp *intel_dp, int mode) > > > > { > > > > struct drm_i915_private *i915 = dp_to_i915(intel_dp); > > > > + u8 edp_oui[] = { 0x00, 0xaa, 0x01 }; > > > > > > what are these values? > > We in i915 typically use the OUI number for adding any eDP specific > quirks. I have seen these getting spit out in the dmesg log at thebeginning. > AFAIK It is some kind of OEM identification number for a display panel. Are you sure you're not confusing this with the sink OUI btw? We're setting the source OUI (so, identifying ourselves to the display panel instead of the other way around) here to tell the panel to expose the Intel specific backlight controls. My assumption is the { 0x00, 0xaa, 0x01 } is some Intel driver OUI, it's just I'm not really sure where it comes from other then the patch I linked to down below > > Manasi > > I wish I knew, my assumption is this is the OUI that Intel's GPU driver > > uses on > > other platforms, but I don't have any documentation mentioning this (in > > fact, > > the few documents I do have on this backlight interface don't seem to make > > any > > mention of it). I only managed to find this when looking through the last > > attempt someone did at adding support for this backlight interface: > > > > https://patchwork.freedesktop.org/patch/334989/ > > > > I think it should be fairly safe to write, as I know nouveau always > > programs a > > source OUI (we don't do it from our driver, but nvidia hardware seems to > > do it > > automatically) and I don't believe I've seen it ever change any behavior > > besides > > making things appear in the 0x300-0x3ff register range. > > > > AFAICT though, the backlight interface won't advertise itself without this > > being > > set early on. If you could find anyone from Intel who knows more about it > > though > > I'd definitely appreciate it (and just in general for the rest of the > > patch > > series as well) > > > > > > int ret, i; > > > > > > > > /* Should have a valid DPCD by this point */ > > > > @@ -3443,6 +3444,14 @@ 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)) { > > > > + ret = drm_dp_dpcd_write(&intel_dp->aux, > > > > DP_SOURCE_OUI, > > > > edp_oui, > > > > + sizeof(edp_oui)); > > > > + if (ret < 0) > > > > + drm_err(&i915->drm, "Failed to write > > > > eDP source > > > > OUI\n"); > > > > + } > > > > + > > > > /* > > > > * When turning on, we need to retry for 1ms to give > > > > the sink > > > > * time to wake up. > > > > @@ -4530,6 +4539,23 @@ static void intel_dp_get_dsc_sink_cap(struct > > > > intel_dp > > > > *intel_dp) > > > > } > > > > } > > > > > > > > +static void > > > > +intel_edp_init_source_oui(struct intel_dp *intel_dp) > > > > +{ > > > > + struct drm_i915_private *i915 = dp_to_i915(intel_dp); > > > > + u8 oui[] = { 0x00, 0xaa, 0x01 }; > > > > + u8 buf[3] = { 0 }; > > > > + > > > > + 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"); > > > > +} > > > > + > > > > static bool > > > > intel_edp_init_dpcd(struct intel_dp *intel_dp) > > > > { > > > > @@ -4607,6 +4633,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); > > > > > > > > + /* > > > > + * 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); > > > > > > I believe we should restrict this to the supported platforms: cfl, whl, > > > cml, > > > icl, tgl > > > no? > > > > > > > + > > > > return true; > > > > } > > > > > > > > -- > > > > 2.26.2 > > > > > > > > _______________________________________________ > > > > dri-devel mailing list > > > > dri-devel@lists.freedesktop.org > > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel > > -- > > Sincerely, > > Lyude Paul (she/her) > > Software Engineer at Red Hat > >
On Tue, 15 Sep 2020, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote: > On Tue, Sep 15, 2020 at 01:29:35PM -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. (For whatever reason I didn't receive the original message.) >> >> 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 | 32 +++++++++++++++++++++++++ >> 1 file changed, 32 insertions(+) >> >> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c >> index 4bd10456ad188..b591672ec4eab 100644 >> --- a/drivers/gpu/drm/i915/display/intel_dp.c >> +++ b/drivers/gpu/drm/i915/display/intel_dp.c >> @@ -3428,6 +3428,7 @@ void intel_dp_sink_set_decompression_state(struct intel_dp *intel_dp, >> void intel_dp_sink_dpms(struct intel_dp *intel_dp, int mode) >> { >> struct drm_i915_private *i915 = dp_to_i915(intel_dp); >> + u8 edp_oui[] = { 0x00, 0xaa, 0x01 }; > > what are these values? An OUI lookup confirms these are Intel OUI. > >> int ret, i; >> >> /* Should have a valid DPCD by this point */ >> @@ -3443,6 +3444,14 @@ 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)) { >> + ret = drm_dp_dpcd_write(&intel_dp->aux, DP_SOURCE_OUI, edp_oui, >> + sizeof(edp_oui)); >> + if (ret < 0) >> + drm_err(&i915->drm, "Failed to write eDP source OUI\n"); >> + } >> + >> /* >> * When turning on, we need to retry for 1ms to give the sink >> * time to wake up. >> @@ -4530,6 +4539,23 @@ static void intel_dp_get_dsc_sink_cap(struct intel_dp *intel_dp) >> } >> } >> >> +static void >> +intel_edp_init_source_oui(struct intel_dp *intel_dp) >> +{ >> + struct drm_i915_private *i915 = dp_to_i915(intel_dp); >> + u8 oui[] = { 0x00, 0xaa, 0x01 }; >> + u8 buf[3] = { 0 }; >> + >> + 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"); >> +} Maybe add this function with a parameter to force write or write only if necessary, and call from both places that set source OUI? >> + >> static bool >> intel_edp_init_dpcd(struct intel_dp *intel_dp) >> { >> @@ -4607,6 +4633,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); >> >> + /* >> + * 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); > > I believe we should restrict this to the supported platforms: cfl, whl, cml, icl, tgl > no? Mmh, this just exposes sink behaviour that I think can be supported by any platform. I don't understand the notion of "supported platforms" here. > >> + >> return true; >> } >> >> -- >> 2.26.2 >> >> _______________________________________________ >> dri-devel mailing list >> dri-devel@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/dri-devel > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Wed, 2020-09-16 at 10:43 +0300, Jani Nikula wrote: > On Tue, 15 Sep 2020, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote: > > On Tue, Sep 15, 2020 at 01:29:35PM -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. > > (For whatever reason I didn't receive the original message.) > > > > 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 | 32 +++++++++++++++++++++++++ > > > 1 file changed, 32 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c > > > b/drivers/gpu/drm/i915/display/intel_dp.c > > > index 4bd10456ad188..b591672ec4eab 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_dp.c > > > +++ b/drivers/gpu/drm/i915/display/intel_dp.c > > > @@ -3428,6 +3428,7 @@ void intel_dp_sink_set_decompression_state(struct > > > intel_dp *intel_dp, > > > void intel_dp_sink_dpms(struct intel_dp *intel_dp, int mode) > > > { > > > struct drm_i915_private *i915 = dp_to_i915(intel_dp); > > > + u8 edp_oui[] = { 0x00, 0xaa, 0x01 }; > > > > what are these values? > > An OUI lookup confirms these are Intel OUI. Thanks for the confirmation! > > > > int ret, i; > > > > > > /* Should have a valid DPCD by this point */ > > > @@ -3443,6 +3444,14 @@ 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)) { > > > + ret = drm_dp_dpcd_write(&intel_dp->aux, DP_SOURCE_OUI, > > > edp_oui, > > > + sizeof(edp_oui)); > > > + if (ret < 0) > > > + drm_err(&i915->drm, "Failed to write eDP source > > > OUI\n"); > > > + } > > > + > > > /* > > > * When turning on, we need to retry for 1ms to give the sink > > > * time to wake up. > > > @@ -4530,6 +4539,23 @@ static void intel_dp_get_dsc_sink_cap(struct > > > intel_dp *intel_dp) > > > } > > > } > > > > > > +static void > > > +intel_edp_init_source_oui(struct intel_dp *intel_dp) > > > +{ > > > + struct drm_i915_private *i915 = dp_to_i915(intel_dp); > > > + u8 oui[] = { 0x00, 0xaa, 0x01 }; > > > + u8 buf[3] = { 0 }; > > > + > > > + 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"); > > > +} > > Maybe add this function with a parameter to force write or write only if > necessary, and call from both places that set source OUI? > > > > + > > > static bool > > > intel_edp_init_dpcd(struct intel_dp *intel_dp) > > > { > > > @@ -4607,6 +4633,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); > > > > > > + /* > > > + * 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); > > > > I believe we should restrict this to the supported platforms: cfl, whl, cml, > > icl, tgl > > no? > > Mmh, this just exposes sink behaviour that I think can be supported by > any platform. I don't understand the notion of "supported platforms" > here. Probably because the spec sheets that we have on this seem to suggest that this is new for particular platforms, and Intel seems to also additionally move a bit away from some of the interfaces exposed here onto actual VESA standards starting with icl and tgl. I would be fine with adding this, but I'm not really sure it's needed here either unless we want to stop using Intel backlight control interfaces for later hardware generations at some point in the future. > > > > + > > > return true; > > > } > > > > > > -- > > > 2.26.2 > > > > > > _______________________________________________ > > > dri-devel mailing list > > > dri-devel@lists.freedesktop.org > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c index 4bd10456ad188..b591672ec4eab 100644 --- a/drivers/gpu/drm/i915/display/intel_dp.c +++ b/drivers/gpu/drm/i915/display/intel_dp.c @@ -3428,6 +3428,7 @@ void intel_dp_sink_set_decompression_state(struct intel_dp *intel_dp, void intel_dp_sink_dpms(struct intel_dp *intel_dp, int mode) { struct drm_i915_private *i915 = dp_to_i915(intel_dp); + u8 edp_oui[] = { 0x00, 0xaa, 0x01 }; int ret, i; /* Should have a valid DPCD by this point */ @@ -3443,6 +3444,14 @@ 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)) { + ret = drm_dp_dpcd_write(&intel_dp->aux, DP_SOURCE_OUI, edp_oui, + sizeof(edp_oui)); + if (ret < 0) + drm_err(&i915->drm, "Failed to write eDP source OUI\n"); + } + /* * When turning on, we need to retry for 1ms to give the sink * time to wake up. @@ -4530,6 +4539,23 @@ static void intel_dp_get_dsc_sink_cap(struct intel_dp *intel_dp) } } +static void +intel_edp_init_source_oui(struct intel_dp *intel_dp) +{ + struct drm_i915_private *i915 = dp_to_i915(intel_dp); + u8 oui[] = { 0x00, 0xaa, 0x01 }; + u8 buf[3] = { 0 }; + + 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"); +} + static bool intel_edp_init_dpcd(struct intel_dp *intel_dp) { @@ -4607,6 +4633,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); + /* + * 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); + return true; }
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. 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 | 32 +++++++++++++++++++++++++ 1 file changed, 32 insertions(+)