Re: [Intel-gfx] [PATCH v3 1/2] drm/i915/opregion: add support for mailbox #5 EDID

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi,

On Tue, Aug 31, 2021 at 11:30 AM Jani Nikula <jani.nikula@xxxxxxxxx> wrote:
>
> On Tue, 17 Aug 2021, Anisse Astier <anisse@xxxxxxxxx> 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 by adding
> > the EDID to the list of available modes on the connector, and use it for
> > eDP when available.
> >
> > If a panel's EDID is broken, there may be an override EDID set in the
> > ACPI OpRegion mailbox #5. Use it if available.
> >
> > Fixes the GPD Win Max laptop display, which seems to only use this
> > mechanism to provide a proper EDID for its eDP screen. It would have
> > been better to provide the EDID through the ACPI _DDC method instead, to
> > have a more generic solution, but it seems the designers of this system
> > did not consider it, and shipped the firmware without it.
>
> The question is whether the opregion EDID should be used for override or
> fallback. The patch at hand is kind of neither, it just unconditionally
> adds the modes from the opregion EDID to the connector. For your
> display, they apparently end up also being the only mode(s), with one of
> them being used as the fixed mode. (Otherwise the orientation quirk
> wouldn't work.)

Yes, I agree it should be a fallback, at least for this hardware.

>
> What does drm_get_edid() return for you? Maybe we should do something
> like this instead:
>
>         mutex_lock(&dev->mode_config.mutex);
>         edid = drm_get_edid(connector, &intel_dp->aux.ddc);
> +       if (!edid)
> +               edid = intel_opregion_get_edid(intel_connector);
>         if (edid) {
>                 if (drm_add_edid_modes(connector, edid)) {
>                         drm_connector_update_edid_property(connector, edid);

Yes, I just tried this, it works well since drm_get_edid() does not
return an EDID.

>
> This way we'll actually use all the other bits in the EDID, not just the
> modes. And if it needs to become override rather than fallback, the
> solution is:
>
>         mutex_lock(&dev->mode_config.mutex);
> -       edid = drm_get_edid(connector, &intel_dp->aux.ddc);
> +       edid = intel_opregion_get_edid(intel_connector);
> +       if (!edid)
> +               edid = drm_get_edid(connector, &intel_dp->aux.ddc);
>         if (edid) {
>                 if (drm_add_edid_modes(connector, edid)) {
>                         drm_connector_update_edid_property(connector, edid);
>
> In any case, I think it's just plain wrong to use both the display and
> opregion EDIDs at the same time. It's probably all around safer to start
> with fallback.

It will be in the next iteration.

>
> Please find some further comments inline.
>
> > Based on original patch series by: Jani Nikula <jani.nikula@xxxxxxxxx>
> > https://patchwork.kernel.org/project/intel-gfx/patch/20200828061941.17051-1-jani.nikula@xxxxxxxxx/
> >
> > Changes since Jani Nikula's series:
> >  - EDID is copied and validated with drm_edid_is_valid
> >  - Mode is now added via drm_add_edid_modes instead of using override
> >    mechanism
> >  - squashed the two patches
> >
> > Cc: Jani Nikula <jani.nikula@xxxxxxxxx>
> > Cc: Uma Shankar <uma.shankar@xxxxxxxxx>
> > Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > Signed-off-by: Anisse Astier <anisse@xxxxxxxxx>
> > ---
> >  drivers/gpu/drm/i915/display/intel_dp.c       |  3 +
> >  drivers/gpu/drm/i915/display/intel_opregion.c | 69 ++++++++++++++++++-
> >  drivers/gpu/drm/i915/display/intel_opregion.h |  8 +++
> >  3 files changed, 79 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> > index 75d4ebc66941..f9254c0df1a2 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > @@ -5183,6 +5183,9 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
> >               goto out_vdd_off;
> >       }
> >
> > +     /* Set up override EDID, if any, from ACPI OpRegion */
> > +     intel_opregion_edid_probe(intel_connector);
> > +
> >       mutex_lock(&dev->mode_config.mutex);
> >       edid = drm_get_edid(connector, &intel_dp->aux.ddc);
> >       if (edid) {
> > diff --git a/drivers/gpu/drm/i915/display/intel_opregion.c b/drivers/gpu/drm/i915/display/intel_opregion.c
> > index 3855fba70980..b1b87ed758ba 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;
> > @@ -1037,6 +1041,68 @@ intel_opregion_get_panel_type(struct drm_i915_private *dev_priv)
> >       return ret - 1;
> >  }
> >
> > +/**
> > + * intel_opregion_edid_probe - Add EDID from ACPI OpRegion mailbox #5
> > + * @intel_connector: eDP connector
> > + *
> > + * This reads the ACPI Opregion mailbox #5 to extract the EDID that is passed
> > + * to it.
> > + *
> > + * Will take a lock on the DRM mode_config to add the EDID; make sure it isn't
> > + * called with lock taken.
> > + *
> > + */
> > +void intel_opregion_edid_probe(struct intel_connector *intel_connector)
>
> With what I proposed this would become:
>
> struct edid *intel_opregion_get_edid(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;
> > +     struct edid *new_edid;
> > +     int len, ret, num;
> > +
> > +     if (!opregion->asle_ext || connector->override_edid)
>
> I don't think ->override_edid can actually be true at this point.
Ok.

>
> > +             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;
> > +
> > +     if (len < EDID_LENGTH * (1 + edid->extensions)) {
> > +             drm_dbg_kms(&i915->drm, "Invalid EDID in ACPI OpRegion (Mailbox #5)\n");
> > +             return;
> > +     }
> > +     new_edid = drm_edid_duplicate(edid);
> > +     if (!new_edid) {
> > +             drm_err(&i915->drm, "Cannot duplicate EDID\n");
>
> In general, it's useless to try to debug print kmalloc errors.
>
> > +             return;
> > +     }
> > +     if (!drm_edid_is_valid(new_edid)) {
> > +             kfree(new_edid);
> > +             drm_dbg_kms(&i915->drm, "Cannot validate EDID in ACPI OpRegion (Mailbox #5)\n");
>
> Please just say the EDID is invalid. "Cannot validate" seems like we
> weren't able to check.
Ok.

>
> > +             return;
> > +     }
> > +
> > +     ret = drm_connector_update_edid_property(connector, new_edid);
> > +     if (ret) {
> > +             kfree(new_edid);
> > +             return;
> > +     }
> > +
> > +     mutex_lock(&connector->dev->mode_config.mutex);
> > +     num = drm_add_edid_modes(connector, new_edid);
> > +     mutex_unlock(&connector->dev->mode_config.mutex);
>
> The above parts would go away with my proposal.
Ok.

>
> > +
> > +     drm_dbg_kms(&i915->drm, "Using OpRegion EDID for [CONNECTOR:%d:%s], added %d mode(s)\n",
> > +                 connector->base.id, connector->name, num);
>
> It's customary to have [CONNECTOR:%d:%s] as the first thing in the debug
> log; it's easier to look for in dmesg.
Ok.

>
> > +}
> > +
> >  void intel_opregion_register(struct drm_i915_private *i915)
> >  {
> >       struct intel_opregion *opregion = &i915->opregion;
> > @@ -1130,6 +1196,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..c1ecfcbb6f55 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_probe(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_probe(struct intel_connector *connector)
> > +{
> > +}
> > +
>
> static inline
Noted.
>
> >  #endif /* CONFIG_ACPI */
> >
> >  #endif
>
> --
> Jani Nikula, Intel Open Source Graphics Center

Thanks a lot for your review time.

Anisse




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux