| Message ID | 20200828061941.17051-1-jani.nikula@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] drm/i915/opregion: add support for mailbox #5 EDID | expand |
On Fri, 28 Aug 2020, Jani Nikula <jani.nikula@intel.com> wrote: > The ACPI OpRegion Mailbox #5 ASLE extension may contain an EDID to be > used for the embedded display. Add support for using it via the EDID > override mechanism. > > Note that the override EDID may be later reset or changed via debugfs, > as usual. > > Cc: Uma Shankar <uma.shankar@intel.com> > Signed-off-by: Jani Nikula <jani.nikula@intel.com> > --- > drivers/gpu/drm/i915/display/intel_opregion.c | 46 ++++++++++++++++++- > drivers/gpu/drm/i915/display/intel_opregion.h | 8 ++++ > 2 files changed, 53 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_opregion.c b/drivers/gpu/drm/i915/display/intel_opregion.c > index de995362f428..13485969fafa 100644 > --- a/drivers/gpu/drm/i915/display/intel_opregion.c > +++ b/drivers/gpu/drm/i915/display/intel_opregion.c > @@ -196,6 +196,8 @@ struct opregion_asle_ext { > #define ASLE_IUER_WINDOWS_BTN (1 << 1) > #define ASLE_IUER_POWER_BTN (1 << 0) > > +#define ASLE_PHED_EDID_VALID_MASK 0x3 > + > /* Software System Control Interrupt (SWSCI) */ > #define SWSCI_SCIC_INDICATOR (1 << 0) > #define SWSCI_SCIC_MAIN_FUNCTION_SHIFT 1 > @@ -909,8 +911,10 @@ int intel_opregion_setup(struct drm_i915_private *dev_priv) > opregion->asle->ardy = ASLE_ARDY_NOT_READY; > } > > - if (mboxes & MBOX_ASLE_EXT) > + if (mboxes & MBOX_ASLE_EXT) { > drm_dbg(&dev_priv->drm, "ASLE extension supported\n"); > + opregion->asle_ext = base + OPREGION_ASLE_EXT_OFFSET; > + } > > if (intel_load_vbt_firmware(dev_priv) == 0) > goto out; > @@ -1041,6 +1045,45 @@ intel_opregion_get_panel_type(struct drm_i915_private *dev_priv) > return ret - 1; > } > > +void intel_opregion_edid_override(struct intel_connector *intel_connector) > +{ > + struct drm_connector *connector = &intel_connector->base; > + struct drm_i915_private *i915 = to_i915(connector->dev); > + struct intel_opregion *opregion = &i915->opregion; > + const void *in_edid; > + const struct edid *edid; > + int len, ret; > + > + if (!opregion->asle_ext) > + return; > + > + in_edid = opregion->asle_ext->bddc; > + > + /* Validity corresponds to number of 128-byte blocks */ > + len = (opregion->asle_ext->phed & ASLE_PHED_EDID_VALID_MASK) * 128; > + if (!len || !memchr_inv(in_edid, 0, len)) > + return; > + > + edid = in_edid; > + > + /* > + * FIXME: Might also check drm_edid_is_valid(edid) here but that > + * requires mutable edid. > + */ > + if (len < EDID_LENGTH * (1 + edid->extensions)) { > + drm_dbg_kms(&i915->drm, "Invalid EDID in ACPI OpRegion (Mailbox #5)\n"); > + return; > + } > + > + connector->override_edid = false; > + ret = drm_connector_update_edid_property(connector, edid); > + if (ret) > + return; > + This is missing here: connector->override_edid = true; > + drm_dbg_kms(&i915->drm, "Using OpRegion EDID for [CONNECTOR:%d:%s]\n", > + connector->base.id, connector->name); > +} > + > void intel_opregion_register(struct drm_i915_private *i915) > { > struct intel_opregion *opregion = &i915->opregion; > @@ -1131,6 +1174,7 @@ void intel_opregion_unregister(struct drm_i915_private *i915) > opregion->acpi = NULL; > opregion->swsci = NULL; > opregion->asle = NULL; > + opregion->asle_ext = NULL; > opregion->vbt = NULL; > opregion->lid_state = NULL; > } > diff --git a/drivers/gpu/drm/i915/display/intel_opregion.h b/drivers/gpu/drm/i915/display/intel_opregion.h > index 4aa68ffbd30e..b407a0744c40 100644 > --- a/drivers/gpu/drm/i915/display/intel_opregion.h > +++ b/drivers/gpu/drm/i915/display/intel_opregion.h > @@ -29,12 +29,14 @@ > #include <linux/pci.h> > > struct drm_i915_private; > +struct intel_connector; > struct intel_encoder; > > struct opregion_header; > struct opregion_acpi; > struct opregion_swsci; > struct opregion_asle; > +struct opregion_asle_ext; > > struct intel_opregion { > struct opregion_header *header; > @@ -43,6 +45,7 @@ struct intel_opregion { > u32 swsci_gbda_sub_functions; > u32 swsci_sbcb_sub_functions; > struct opregion_asle *asle; > + struct opregion_asle_ext *asle_ext; > void *rvda; > void *vbt_firmware; > const void *vbt; > @@ -71,6 +74,7 @@ int intel_opregion_notify_encoder(struct intel_encoder *intel_encoder, > int intel_opregion_notify_adapter(struct drm_i915_private *dev_priv, > pci_power_t state); > int intel_opregion_get_panel_type(struct drm_i915_private *dev_priv); > +void intel_opregion_edid_override(struct intel_connector *connector); > > #else /* CONFIG_ACPI*/ > > @@ -117,6 +121,10 @@ static inline int intel_opregion_get_panel_type(struct drm_i915_private *dev) > return -ENODEV; > } > > +void intel_opregion_edid_override(struct intel_connector *connector) > +{ > +} > + > #endif /* CONFIG_ACPI */ > > #endif
> -----Original Message----- > From: Jani Nikula <jani.nikula@intel.com> > Sent: Friday, August 28, 2020 11:58 AM > To: intel-gfx@lists.freedesktop.org > Cc: Shankar, Uma <uma.shankar@intel.com> > Subject: Re: [PATCH 1/2] drm/i915/opregion: add support for mailbox #5 EDID > > On Fri, 28 Aug 2020, Jani Nikula <jani.nikula@intel.com> wrote: > > The ACPI OpRegion Mailbox #5 ASLE extension may contain an EDID to be > > used for the embedded display. Add support for using it via the EDID > > override mechanism. > > > > Note that the override EDID may be later reset or changed via debugfs, > > as usual. > > > > Cc: Uma Shankar <uma.shankar@intel.com> > > Signed-off-by: Jani Nikula <jani.nikula@intel.com> > > --- > > drivers/gpu/drm/i915/display/intel_opregion.c | 46 > > ++++++++++++++++++- drivers/gpu/drm/i915/display/intel_opregion.h | > > 8 ++++ > > 2 files changed, 53 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_opregion.c > > b/drivers/gpu/drm/i915/display/intel_opregion.c > > index de995362f428..13485969fafa 100644 > > --- a/drivers/gpu/drm/i915/display/intel_opregion.c > > +++ b/drivers/gpu/drm/i915/display/intel_opregion.c > > @@ -196,6 +196,8 @@ struct opregion_asle_ext { > > #define ASLE_IUER_WINDOWS_BTN (1 << 1) > > #define ASLE_IUER_POWER_BTN (1 << 0) > > > > +#define ASLE_PHED_EDID_VALID_MASK 0x3 > > + > > /* Software System Control Interrupt (SWSCI) */ > > #define SWSCI_SCIC_INDICATOR (1 << 0) > > #define SWSCI_SCIC_MAIN_FUNCTION_SHIFT 1 > > @@ -909,8 +911,10 @@ int intel_opregion_setup(struct drm_i915_private > *dev_priv) > > opregion->asle->ardy = ASLE_ARDY_NOT_READY; > > } > > > > - if (mboxes & MBOX_ASLE_EXT) > > + if (mboxes & MBOX_ASLE_EXT) { > > drm_dbg(&dev_priv->drm, "ASLE extension supported\n"); > > + opregion->asle_ext = base + OPREGION_ASLE_EXT_OFFSET; > > + } > > > > if (intel_load_vbt_firmware(dev_priv) == 0) > > goto out; > > @@ -1041,6 +1045,45 @@ intel_opregion_get_panel_type(struct > drm_i915_private *dev_priv) > > return ret - 1; > > } > > > > +void intel_opregion_edid_override(struct intel_connector > > +*intel_connector) { > > + struct drm_connector *connector = &intel_connector->base; > > + struct drm_i915_private *i915 = to_i915(connector->dev); > > + struct intel_opregion *opregion = &i915->opregion; > > + const void *in_edid; > > + const struct edid *edid; > > + int len, ret; > > + > > + if (!opregion->asle_ext) > > + return; > > + > > + in_edid = opregion->asle_ext->bddc; > > + > > + /* Validity corresponds to number of 128-byte blocks */ > > + len = (opregion->asle_ext->phed & ASLE_PHED_EDID_VALID_MASK) * 128; > > + if (!len || !memchr_inv(in_edid, 0, len)) > > + return; > > + > > + edid = in_edid; > > + > > + /* > > + * FIXME: Might also check drm_edid_is_valid(edid) here but that > > + * requires mutable edid. > > + */ > > + if (len < EDID_LENGTH * (1 + edid->extensions)) { > > + drm_dbg_kms(&i915->drm, "Invalid EDID in ACPI OpRegion > (Mailbox #5)\n"); > > + return; > > + } > > + > > + connector->override_edid = false; > > + ret = drm_connector_update_edid_property(connector, edid); > > + if (ret) > > + return; > > + > > This is missing here: > > connector->override_edid = true; Yeah with this added, changes look good to me Reviewed-by: Uma Shankar <uma.shankar@intel.com> > > + drm_dbg_kms(&i915->drm, "Using OpRegion EDID for > [CONNECTOR:%d:%s]\n", > > + connector->base.id, connector->name); } > > + > > void intel_opregion_register(struct drm_i915_private *i915) { > > struct intel_opregion *opregion = &i915->opregion; @@ -1131,6 > > +1174,7 @@ void intel_opregion_unregister(struct drm_i915_private *i915) > > opregion->acpi = NULL; > > opregion->swsci = NULL; > > opregion->asle = NULL; > > + opregion->asle_ext = NULL; > > opregion->vbt = NULL; > > opregion->lid_state = NULL; > > } > > diff --git a/drivers/gpu/drm/i915/display/intel_opregion.h > > b/drivers/gpu/drm/i915/display/intel_opregion.h > > index 4aa68ffbd30e..b407a0744c40 100644 > > --- a/drivers/gpu/drm/i915/display/intel_opregion.h > > +++ b/drivers/gpu/drm/i915/display/intel_opregion.h > > @@ -29,12 +29,14 @@ > > #include <linux/pci.h> > > > > struct drm_i915_private; > > +struct intel_connector; > > struct intel_encoder; > > > > struct opregion_header; > > struct opregion_acpi; > > struct opregion_swsci; > > struct opregion_asle; > > +struct opregion_asle_ext; > > > > struct intel_opregion { > > struct opregion_header *header; > > @@ -43,6 +45,7 @@ struct intel_opregion { > > u32 swsci_gbda_sub_functions; > > u32 swsci_sbcb_sub_functions; > > struct opregion_asle *asle; > > + struct opregion_asle_ext *asle_ext; > > void *rvda; > > void *vbt_firmware; > > const void *vbt; > > @@ -71,6 +74,7 @@ int intel_opregion_notify_encoder(struct > > intel_encoder *intel_encoder, int intel_opregion_notify_adapter(struct > drm_i915_private *dev_priv, > > pci_power_t state); > > int intel_opregion_get_panel_type(struct drm_i915_private *dev_priv); > > +void intel_opregion_edid_override(struct intel_connector *connector); > > > > #else /* CONFIG_ACPI*/ > > > > @@ -117,6 +121,10 @@ static inline int intel_opregion_get_panel_type(struct > drm_i915_private *dev) > > return -ENODEV; > > } > > > > +void intel_opregion_edid_override(struct intel_connector *connector) > > +{ } > > + > > #endif /* CONFIG_ACPI */ > > > > #endif > > -- > Jani Nikula, Intel Open Source Graphics Center
On Fri, Aug 28, 2020 at 09:19:40AM +0300, Jani Nikula wrote: > The ACPI OpRegion Mailbox #5 ASLE extension may contain an EDID to be > used for the embedded display. Add support for using it via the EDID > override mechanism. Abusing the override for this feels a bit odd. Also I have a vague recollection that there was perhaps some linkage between the mailbox and the ACPI _DDC stuff: git://github.com/vsyrjala/linux.git acpi_edid > > Note that the override EDID may be later reset or changed via debugfs, > as usual. > > Cc: Uma Shankar <uma.shankar@intel.com> > Signed-off-by: Jani Nikula <jani.nikula@intel.com> > --- > drivers/gpu/drm/i915/display/intel_opregion.c | 46 ++++++++++++++++++- > drivers/gpu/drm/i915/display/intel_opregion.h | 8 ++++ > 2 files changed, 53 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_opregion.c b/drivers/gpu/drm/i915/display/intel_opregion.c > index de995362f428..13485969fafa 100644 > --- a/drivers/gpu/drm/i915/display/intel_opregion.c > +++ b/drivers/gpu/drm/i915/display/intel_opregion.c > @@ -196,6 +196,8 @@ struct opregion_asle_ext { > #define ASLE_IUER_WINDOWS_BTN (1 << 1) > #define ASLE_IUER_POWER_BTN (1 << 0) > > +#define ASLE_PHED_EDID_VALID_MASK 0x3 > + > /* Software System Control Interrupt (SWSCI) */ > #define SWSCI_SCIC_INDICATOR (1 << 0) > #define SWSCI_SCIC_MAIN_FUNCTION_SHIFT 1 > @@ -909,8 +911,10 @@ int intel_opregion_setup(struct drm_i915_private *dev_priv) > opregion->asle->ardy = ASLE_ARDY_NOT_READY; > } > > - if (mboxes & MBOX_ASLE_EXT) > + if (mboxes & MBOX_ASLE_EXT) { > drm_dbg(&dev_priv->drm, "ASLE extension supported\n"); > + opregion->asle_ext = base + OPREGION_ASLE_EXT_OFFSET; > + } > > if (intel_load_vbt_firmware(dev_priv) == 0) > goto out; > @@ -1041,6 +1045,45 @@ intel_opregion_get_panel_type(struct drm_i915_private *dev_priv) > return ret - 1; > } > > +void intel_opregion_edid_override(struct intel_connector *intel_connector) > +{ > + struct drm_connector *connector = &intel_connector->base; > + struct drm_i915_private *i915 = to_i915(connector->dev); > + struct intel_opregion *opregion = &i915->opregion; > + const void *in_edid; > + const struct edid *edid; > + int len, ret; > + > + if (!opregion->asle_ext) > + return; > + > + in_edid = opregion->asle_ext->bddc; > + > + /* Validity corresponds to number of 128-byte blocks */ > + len = (opregion->asle_ext->phed & ASLE_PHED_EDID_VALID_MASK) * 128; > + if (!len || !memchr_inv(in_edid, 0, len)) > + return; > + > + edid = in_edid; > + > + /* > + * FIXME: Might also check drm_edid_is_valid(edid) here but that > + * requires mutable edid. > + */ > + if (len < EDID_LENGTH * (1 + edid->extensions)) { > + drm_dbg_kms(&i915->drm, "Invalid EDID in ACPI OpRegion (Mailbox #5)\n"); > + return; > + } > + > + connector->override_edid = false; > + ret = drm_connector_update_edid_property(connector, edid); > + if (ret) > + return; > + > + drm_dbg_kms(&i915->drm, "Using OpRegion EDID for [CONNECTOR:%d:%s]\n", > + connector->base.id, connector->name); > +} > + > void intel_opregion_register(struct drm_i915_private *i915) > { > struct intel_opregion *opregion = &i915->opregion; > @@ -1131,6 +1174,7 @@ void intel_opregion_unregister(struct drm_i915_private *i915) > opregion->acpi = NULL; > opregion->swsci = NULL; > opregion->asle = NULL; > + opregion->asle_ext = NULL; > opregion->vbt = NULL; > opregion->lid_state = NULL; > } > diff --git a/drivers/gpu/drm/i915/display/intel_opregion.h b/drivers/gpu/drm/i915/display/intel_opregion.h > index 4aa68ffbd30e..b407a0744c40 100644 > --- a/drivers/gpu/drm/i915/display/intel_opregion.h > +++ b/drivers/gpu/drm/i915/display/intel_opregion.h > @@ -29,12 +29,14 @@ > #include <linux/pci.h> > > struct drm_i915_private; > +struct intel_connector; > struct intel_encoder; > > struct opregion_header; > struct opregion_acpi; > struct opregion_swsci; > struct opregion_asle; > +struct opregion_asle_ext; > > struct intel_opregion { > struct opregion_header *header; > @@ -43,6 +45,7 @@ struct intel_opregion { > u32 swsci_gbda_sub_functions; > u32 swsci_sbcb_sub_functions; > struct opregion_asle *asle; > + struct opregion_asle_ext *asle_ext; > void *rvda; > void *vbt_firmware; > const void *vbt; > @@ -71,6 +74,7 @@ int intel_opregion_notify_encoder(struct intel_encoder *intel_encoder, > int intel_opregion_notify_adapter(struct drm_i915_private *dev_priv, > pci_power_t state); > int intel_opregion_get_panel_type(struct drm_i915_private *dev_priv); > +void intel_opregion_edid_override(struct intel_connector *connector); > > #else /* CONFIG_ACPI*/ > > @@ -117,6 +121,10 @@ static inline int intel_opregion_get_panel_type(struct drm_i915_private *dev) > return -ENODEV; > } > > +void intel_opregion_edid_override(struct intel_connector *connector) > +{ > +} > + > #endif /* CONFIG_ACPI */ > > #endif > -- > 2.20.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Mon, 31 Aug 2020, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > On Fri, Aug 28, 2020 at 09:19:40AM +0300, Jani Nikula wrote: >> The ACPI OpRegion Mailbox #5 ASLE extension may contain an EDID to be >> used for the embedded display. Add support for using it via the EDID >> override mechanism. > > Abusing the override for this feels a bit odd. It's the least intrusive way to make this work across the drm and driver EDID code that I could think of. BR, Jani. > > Also I have a vague recollection that there was perhaps some > linkage between the mailbox and the ACPI _DDC stuff: > git://github.com/vsyrjala/linux.git acpi_edid > >> >> Note that the override EDID may be later reset or changed via debugfs, >> as usual. >> >> Cc: Uma Shankar <uma.shankar@intel.com> >> Signed-off-by: Jani Nikula <jani.nikula@intel.com> >> --- >> drivers/gpu/drm/i915/display/intel_opregion.c | 46 ++++++++++++++++++- >> drivers/gpu/drm/i915/display/intel_opregion.h | 8 ++++ >> 2 files changed, 53 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/i915/display/intel_opregion.c b/drivers/gpu/drm/i915/display/intel_opregion.c >> index de995362f428..13485969fafa 100644 >> --- a/drivers/gpu/drm/i915/display/intel_opregion.c >> +++ b/drivers/gpu/drm/i915/display/intel_opregion.c >> @@ -196,6 +196,8 @@ struct opregion_asle_ext { >> #define ASLE_IUER_WINDOWS_BTN (1 << 1) >> #define ASLE_IUER_POWER_BTN (1 << 0) >> >> +#define ASLE_PHED_EDID_VALID_MASK 0x3 >> + >> /* Software System Control Interrupt (SWSCI) */ >> #define SWSCI_SCIC_INDICATOR (1 << 0) >> #define SWSCI_SCIC_MAIN_FUNCTION_SHIFT 1 >> @@ -909,8 +911,10 @@ int intel_opregion_setup(struct drm_i915_private *dev_priv) >> opregion->asle->ardy = ASLE_ARDY_NOT_READY; >> } >> >> - if (mboxes & MBOX_ASLE_EXT) >> + if (mboxes & MBOX_ASLE_EXT) { >> drm_dbg(&dev_priv->drm, "ASLE extension supported\n"); >> + opregion->asle_ext = base + OPREGION_ASLE_EXT_OFFSET; >> + } >> >> if (intel_load_vbt_firmware(dev_priv) == 0) >> goto out; >> @@ -1041,6 +1045,45 @@ intel_opregion_get_panel_type(struct drm_i915_private *dev_priv) >> return ret - 1; >> } >> >> +void intel_opregion_edid_override(struct intel_connector *intel_connector) >> +{ >> + struct drm_connector *connector = &intel_connector->base; >> + struct drm_i915_private *i915 = to_i915(connector->dev); >> + struct intel_opregion *opregion = &i915->opregion; >> + const void *in_edid; >> + const struct edid *edid; >> + int len, ret; >> + >> + if (!opregion->asle_ext) >> + return; >> + >> + in_edid = opregion->asle_ext->bddc; >> + >> + /* Validity corresponds to number of 128-byte blocks */ >> + len = (opregion->asle_ext->phed & ASLE_PHED_EDID_VALID_MASK) * 128; >> + if (!len || !memchr_inv(in_edid, 0, len)) >> + return; >> + >> + edid = in_edid; >> + >> + /* >> + * FIXME: Might also check drm_edid_is_valid(edid) here but that >> + * requires mutable edid. >> + */ >> + if (len < EDID_LENGTH * (1 + edid->extensions)) { >> + drm_dbg_kms(&i915->drm, "Invalid EDID in ACPI OpRegion (Mailbox #5)\n"); >> + return; >> + } >> + >> + connector->override_edid = false; >> + ret = drm_connector_update_edid_property(connector, edid); >> + if (ret) >> + return; >> + >> + drm_dbg_kms(&i915->drm, "Using OpRegion EDID for [CONNECTOR:%d:%s]\n", >> + connector->base.id, connector->name); >> +} >> + >> void intel_opregion_register(struct drm_i915_private *i915) >> { >> struct intel_opregion *opregion = &i915->opregion; >> @@ -1131,6 +1174,7 @@ void intel_opregion_unregister(struct drm_i915_private *i915) >> opregion->acpi = NULL; >> opregion->swsci = NULL; >> opregion->asle = NULL; >> + opregion->asle_ext = NULL; >> opregion->vbt = NULL; >> opregion->lid_state = NULL; >> } >> diff --git a/drivers/gpu/drm/i915/display/intel_opregion.h b/drivers/gpu/drm/i915/display/intel_opregion.h >> index 4aa68ffbd30e..b407a0744c40 100644 >> --- a/drivers/gpu/drm/i915/display/intel_opregion.h >> +++ b/drivers/gpu/drm/i915/display/intel_opregion.h >> @@ -29,12 +29,14 @@ >> #include <linux/pci.h> >> >> struct drm_i915_private; >> +struct intel_connector; >> struct intel_encoder; >> >> struct opregion_header; >> struct opregion_acpi; >> struct opregion_swsci; >> struct opregion_asle; >> +struct opregion_asle_ext; >> >> struct intel_opregion { >> struct opregion_header *header; >> @@ -43,6 +45,7 @@ struct intel_opregion { >> u32 swsci_gbda_sub_functions; >> u32 swsci_sbcb_sub_functions; >> struct opregion_asle *asle; >> + struct opregion_asle_ext *asle_ext; >> void *rvda; >> void *vbt_firmware; >> const void *vbt; >> @@ -71,6 +74,7 @@ int intel_opregion_notify_encoder(struct intel_encoder *intel_encoder, >> int intel_opregion_notify_adapter(struct drm_i915_private *dev_priv, >> pci_power_t state); >> int intel_opregion_get_panel_type(struct drm_i915_private *dev_priv); >> +void intel_opregion_edid_override(struct intel_connector *connector); >> >> #else /* CONFIG_ACPI*/ >> >> @@ -117,6 +121,10 @@ static inline int intel_opregion_get_panel_type(struct drm_i915_private *dev) >> return -ENODEV; >> } >> >> +void intel_opregion_edid_override(struct intel_connector *connector) >> +{ >> +} >> + >> #endif /* CONFIG_ACPI */ >> >> #endif >> -- >> 2.20.1 >> >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Mon, 31 Aug 2020, Jani Nikula <jani.nikula@intel.com> wrote: > On Mon, 31 Aug 2020, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: >> On Fri, Aug 28, 2020 at 09:19:40AM +0300, Jani Nikula wrote: >>> The ACPI OpRegion Mailbox #5 ASLE extension may contain an EDID to be >>> used for the embedded display. Add support for using it via the EDID >>> override mechanism. >> >> Abusing the override for this feels a bit odd. > > It's the least intrusive way to make this work across the drm and driver > EDID code that I could think of. > > BR, > Jani. > > >> >> Also I have a vague recollection that there was perhaps some >> linkage between the mailbox and the ACPI _DDC stuff: >> git://github.com/vsyrjala/linux.git acpi_edid Only looked at this now. The patch at hand is for actually overriding the EDID from the panel, because the panel EDID is readable but bogus. I have no idea how the ACPI _DDC stuff would work in this case. Would it return the EDID from the panel or from mailbox #5 or something completely different? Who knows. Using drm_do_get_edid() would of course be doable for reading mailbox #5 directly as well, but you'd have to make that the "primary" method and fall back to the usual drm_get_edid(). Note that this completely prevents you from ever reading the actual panel EDID. Using edid_override lets you get at the panel EDID too. BR, Jani. >> >>> >>> Note that the override EDID may be later reset or changed via debugfs, >>> as usual. >>> >>> Cc: Uma Shankar <uma.shankar@intel.com> >>> Signed-off-by: Jani Nikula <jani.nikula@intel.com> >>> --- >>> drivers/gpu/drm/i915/display/intel_opregion.c | 46 ++++++++++++++++++- >>> drivers/gpu/drm/i915/display/intel_opregion.h | 8 ++++ >>> 2 files changed, 53 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/gpu/drm/i915/display/intel_opregion.c b/drivers/gpu/drm/i915/display/intel_opregion.c >>> index de995362f428..13485969fafa 100644 >>> --- a/drivers/gpu/drm/i915/display/intel_opregion.c >>> +++ b/drivers/gpu/drm/i915/display/intel_opregion.c >>> @@ -196,6 +196,8 @@ struct opregion_asle_ext { >>> #define ASLE_IUER_WINDOWS_BTN (1 << 1) >>> #define ASLE_IUER_POWER_BTN (1 << 0) >>> >>> +#define ASLE_PHED_EDID_VALID_MASK 0x3 >>> + >>> /* Software System Control Interrupt (SWSCI) */ >>> #define SWSCI_SCIC_INDICATOR (1 << 0) >>> #define SWSCI_SCIC_MAIN_FUNCTION_SHIFT 1 >>> @@ -909,8 +911,10 @@ int intel_opregion_setup(struct drm_i915_private *dev_priv) >>> opregion->asle->ardy = ASLE_ARDY_NOT_READY; >>> } >>> >>> - if (mboxes & MBOX_ASLE_EXT) >>> + if (mboxes & MBOX_ASLE_EXT) { >>> drm_dbg(&dev_priv->drm, "ASLE extension supported\n"); >>> + opregion->asle_ext = base + OPREGION_ASLE_EXT_OFFSET; >>> + } >>> >>> if (intel_load_vbt_firmware(dev_priv) == 0) >>> goto out; >>> @@ -1041,6 +1045,45 @@ intel_opregion_get_panel_type(struct drm_i915_private *dev_priv) >>> return ret - 1; >>> } >>> >>> +void intel_opregion_edid_override(struct intel_connector *intel_connector) >>> +{ >>> + struct drm_connector *connector = &intel_connector->base; >>> + struct drm_i915_private *i915 = to_i915(connector->dev); >>> + struct intel_opregion *opregion = &i915->opregion; >>> + const void *in_edid; >>> + const struct edid *edid; >>> + int len, ret; >>> + >>> + if (!opregion->asle_ext) >>> + return; >>> + >>> + in_edid = opregion->asle_ext->bddc; >>> + >>> + /* Validity corresponds to number of 128-byte blocks */ >>> + len = (opregion->asle_ext->phed & ASLE_PHED_EDID_VALID_MASK) * 128; >>> + if (!len || !memchr_inv(in_edid, 0, len)) >>> + return; >>> + >>> + edid = in_edid; >>> + >>> + /* >>> + * FIXME: Might also check drm_edid_is_valid(edid) here but that >>> + * requires mutable edid. >>> + */ >>> + if (len < EDID_LENGTH * (1 + edid->extensions)) { >>> + drm_dbg_kms(&i915->drm, "Invalid EDID in ACPI OpRegion (Mailbox #5)\n"); >>> + return; >>> + } >>> + >>> + connector->override_edid = false; >>> + ret = drm_connector_update_edid_property(connector, edid); >>> + if (ret) >>> + return; >>> + >>> + drm_dbg_kms(&i915->drm, "Using OpRegion EDID for [CONNECTOR:%d:%s]\n", >>> + connector->base.id, connector->name); >>> +} >>> + >>> void intel_opregion_register(struct drm_i915_private *i915) >>> { >>> struct intel_opregion *opregion = &i915->opregion; >>> @@ -1131,6 +1174,7 @@ void intel_opregion_unregister(struct drm_i915_private *i915) >>> opregion->acpi = NULL; >>> opregion->swsci = NULL; >>> opregion->asle = NULL; >>> + opregion->asle_ext = NULL; >>> opregion->vbt = NULL; >>> opregion->lid_state = NULL; >>> } >>> diff --git a/drivers/gpu/drm/i915/display/intel_opregion.h b/drivers/gpu/drm/i915/display/intel_opregion.h >>> index 4aa68ffbd30e..b407a0744c40 100644 >>> --- a/drivers/gpu/drm/i915/display/intel_opregion.h >>> +++ b/drivers/gpu/drm/i915/display/intel_opregion.h >>> @@ -29,12 +29,14 @@ >>> #include <linux/pci.h> >>> >>> struct drm_i915_private; >>> +struct intel_connector; >>> struct intel_encoder; >>> >>> struct opregion_header; >>> struct opregion_acpi; >>> struct opregion_swsci; >>> struct opregion_asle; >>> +struct opregion_asle_ext; >>> >>> struct intel_opregion { >>> struct opregion_header *header; >>> @@ -43,6 +45,7 @@ struct intel_opregion { >>> u32 swsci_gbda_sub_functions; >>> u32 swsci_sbcb_sub_functions; >>> struct opregion_asle *asle; >>> + struct opregion_asle_ext *asle_ext; >>> void *rvda; >>> void *vbt_firmware; >>> const void *vbt; >>> @@ -71,6 +74,7 @@ int intel_opregion_notify_encoder(struct intel_encoder *intel_encoder, >>> int intel_opregion_notify_adapter(struct drm_i915_private *dev_priv, >>> pci_power_t state); >>> int intel_opregion_get_panel_type(struct drm_i915_private *dev_priv); >>> +void intel_opregion_edid_override(struct intel_connector *connector); >>> >>> #else /* CONFIG_ACPI*/ >>> >>> @@ -117,6 +121,10 @@ static inline int intel_opregion_get_panel_type(struct drm_i915_private *dev) >>> return -ENODEV; >>> } >>> >>> +void intel_opregion_edid_override(struct intel_connector *connector) >>> +{ >>> +} >>> + >>> #endif /* CONFIG_ACPI */ >>> >>> #endif >>> -- >>> 2.20.1 >>> >>> _______________________________________________ >>> Intel-gfx mailing list >>> Intel-gfx@lists.freedesktop.org >>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Tue, Sep 01, 2020 at 04:18:19PM +0300, Jani Nikula wrote: > On Mon, 31 Aug 2020, Jani Nikula <jani.nikula@intel.com> wrote: > > On Mon, 31 Aug 2020, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > >> On Fri, Aug 28, 2020 at 09:19:40AM +0300, Jani Nikula wrote: > >>> The ACPI OpRegion Mailbox #5 ASLE extension may contain an EDID to be > >>> used for the embedded display. Add support for using it via the EDID > >>> override mechanism. > >> > >> Abusing the override for this feels a bit odd. > > > > It's the least intrusive way to make this work across the drm and driver > > EDID code that I could think of. > > > > BR, > > Jani. > > > > > >> > >> Also I have a vague recollection that there was perhaps some > >> linkage between the mailbox and the ACPI _DDC stuff: > >> git://github.com/vsyrjala/linux.git acpi_edid > > Only looked at this now. The patch at hand is for actually overriding > the EDID from the panel, because the panel EDID is readable but bogus. Do we have an actual use case for this? The commit msg doesn't say so. > I > have no idea how the ACPI _DDC stuff would work in this case. Would it > return the EDID from the panel or from mailbox #5 or something > completely different? Who knows. > > Using drm_do_get_edid() would of course be doable for reading mailbox #5 > directly as well, but you'd have to make that the "primary" method and > fall back to the usual drm_get_edid(). Note that this completely > prevents you from ever reading the actual panel EDID. Using > edid_override lets you get at the panel EDID too. Might be nice to make .get_edid() a connector vfunc and let each driver implement it however they want. That way the driver would be in total control over the priority of different EDID sources. But haven't really looked at what that would take. Not even sure if a vfunc is totally required as I think most EDID reads should be in some connector specific driver code.
On Wed, 02 Sep 2020, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > On Tue, Sep 01, 2020 at 04:18:19PM +0300, Jani Nikula wrote: >> On Mon, 31 Aug 2020, Jani Nikula <jani.nikula@intel.com> wrote: >> > On Mon, 31 Aug 2020, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: >> >> On Fri, Aug 28, 2020 at 09:19:40AM +0300, Jani Nikula wrote: >> >>> The ACPI OpRegion Mailbox #5 ASLE extension may contain an EDID to be >> >>> used for the embedded display. Add support for using it via the EDID >> >>> override mechanism. >> >> >> >> Abusing the override for this feels a bit odd. >> > >> > It's the least intrusive way to make this work across the drm and driver >> > EDID code that I could think of. >> > >> > BR, >> > Jani. >> > >> > >> >> >> >> Also I have a vague recollection that there was perhaps some >> >> linkage between the mailbox and the ACPI _DDC stuff: >> >> git://github.com/vsyrjala/linux.git acpi_edid >> >> Only looked at this now. The patch at hand is for actually overriding >> the EDID from the panel, because the panel EDID is readable but bogus. > > Do we have an actual use case for this? The commit msg doesn't say so. It's a bit hazy still, but potentially yes, with a need to backport to stable as well. >> I >> have no idea how the ACPI _DDC stuff would work in this case. Would it >> return the EDID from the panel or from mailbox #5 or something >> completely different? Who knows. >> >> Using drm_do_get_edid() would of course be doable for reading mailbox #5 >> directly as well, but you'd have to make that the "primary" method and >> fall back to the usual drm_get_edid(). Note that this completely >> prevents you from ever reading the actual panel EDID. Using >> edid_override lets you get at the panel EDID too. > > Might be nice to make .get_edid() a connector vfunc and let each driver > implement it however they want. That way the driver would be in total > control over the priority of different EDID sources. But haven't really > looked at what that would take. Not even sure if a vfunc is totally > required as I think most EDID reads should be in some connector specific > driver code.
diff --git a/drivers/gpu/drm/i915/display/intel_opregion.c b/drivers/gpu/drm/i915/display/intel_opregion.c index de995362f428..13485969fafa 100644 --- a/drivers/gpu/drm/i915/display/intel_opregion.c +++ b/drivers/gpu/drm/i915/display/intel_opregion.c @@ -196,6 +196,8 @@ struct opregion_asle_ext { #define ASLE_IUER_WINDOWS_BTN (1 << 1) #define ASLE_IUER_POWER_BTN (1 << 0) +#define ASLE_PHED_EDID_VALID_MASK 0x3 + /* Software System Control Interrupt (SWSCI) */ #define SWSCI_SCIC_INDICATOR (1 << 0) #define SWSCI_SCIC_MAIN_FUNCTION_SHIFT 1 @@ -909,8 +911,10 @@ int intel_opregion_setup(struct drm_i915_private *dev_priv) opregion->asle->ardy = ASLE_ARDY_NOT_READY; } - if (mboxes & MBOX_ASLE_EXT) + if (mboxes & MBOX_ASLE_EXT) { drm_dbg(&dev_priv->drm, "ASLE extension supported\n"); + opregion->asle_ext = base + OPREGION_ASLE_EXT_OFFSET; + } if (intel_load_vbt_firmware(dev_priv) == 0) goto out; @@ -1041,6 +1045,45 @@ intel_opregion_get_panel_type(struct drm_i915_private *dev_priv) return ret - 1; } +void intel_opregion_edid_override(struct intel_connector *intel_connector) +{ + struct drm_connector *connector = &intel_connector->base; + struct drm_i915_private *i915 = to_i915(connector->dev); + struct intel_opregion *opregion = &i915->opregion; + const void *in_edid; + const struct edid *edid; + int len, ret; + + if (!opregion->asle_ext) + return; + + in_edid = opregion->asle_ext->bddc; + + /* Validity corresponds to number of 128-byte blocks */ + len = (opregion->asle_ext->phed & ASLE_PHED_EDID_VALID_MASK) * 128; + if (!len || !memchr_inv(in_edid, 0, len)) + return; + + edid = in_edid; + + /* + * FIXME: Might also check drm_edid_is_valid(edid) here but that + * requires mutable edid. + */ + if (len < EDID_LENGTH * (1 + edid->extensions)) { + drm_dbg_kms(&i915->drm, "Invalid EDID in ACPI OpRegion (Mailbox #5)\n"); + return; + } + + connector->override_edid = false; + ret = drm_connector_update_edid_property(connector, edid); + if (ret) + return; + + drm_dbg_kms(&i915->drm, "Using OpRegion EDID for [CONNECTOR:%d:%s]\n", + connector->base.id, connector->name); +} + void intel_opregion_register(struct drm_i915_private *i915) { struct intel_opregion *opregion = &i915->opregion; @@ -1131,6 +1174,7 @@ void intel_opregion_unregister(struct drm_i915_private *i915) opregion->acpi = NULL; opregion->swsci = NULL; opregion->asle = NULL; + opregion->asle_ext = NULL; opregion->vbt = NULL; opregion->lid_state = NULL; } diff --git a/drivers/gpu/drm/i915/display/intel_opregion.h b/drivers/gpu/drm/i915/display/intel_opregion.h index 4aa68ffbd30e..b407a0744c40 100644 --- a/drivers/gpu/drm/i915/display/intel_opregion.h +++ b/drivers/gpu/drm/i915/display/intel_opregion.h @@ -29,12 +29,14 @@ #include <linux/pci.h> struct drm_i915_private; +struct intel_connector; struct intel_encoder; struct opregion_header; struct opregion_acpi; struct opregion_swsci; struct opregion_asle; +struct opregion_asle_ext; struct intel_opregion { struct opregion_header *header; @@ -43,6 +45,7 @@ struct intel_opregion { u32 swsci_gbda_sub_functions; u32 swsci_sbcb_sub_functions; struct opregion_asle *asle; + struct opregion_asle_ext *asle_ext; void *rvda; void *vbt_firmware; const void *vbt; @@ -71,6 +74,7 @@ int intel_opregion_notify_encoder(struct intel_encoder *intel_encoder, int intel_opregion_notify_adapter(struct drm_i915_private *dev_priv, pci_power_t state); int intel_opregion_get_panel_type(struct drm_i915_private *dev_priv); +void intel_opregion_edid_override(struct intel_connector *connector); #else /* CONFIG_ACPI*/ @@ -117,6 +121,10 @@ static inline int intel_opregion_get_panel_type(struct drm_i915_private *dev) return -ENODEV; } +void intel_opregion_edid_override(struct intel_connector *connector) +{ +} + #endif /* CONFIG_ACPI */ #endif
The ACPI OpRegion Mailbox #5 ASLE extension may contain an EDID to be used for the embedded display. Add support for using it via the EDID override mechanism. Note that the override EDID may be later reset or changed via debugfs, as usual. Cc: Uma Shankar <uma.shankar@intel.com> Signed-off-by: Jani Nikula <jani.nikula@intel.com> --- drivers/gpu/drm/i915/display/intel_opregion.c | 46 ++++++++++++++++++- drivers/gpu/drm/i915/display/intel_opregion.h | 8 ++++ 2 files changed, 53 insertions(+), 1 deletion(-)