[Intel-gfx] [PATCH 1/2] drm/i915/opregion: add function to check if headless sku

Tue Jun 7 13:05:10 UTC 2022

On Tue, 2022-06-07 at 10:36 +0300, Jani Nikula wrote:
> On Mon, 06 Jun 2022, "Souza, Jose" <jose.souza at intel.com> wrote:
> > On Mon, 2022-06-06 at 11:16 +0300, Jani Nikula wrote:
> > > On Mon, 06 Jun 2022, "Hogander, Jouni" <jouni.hogander at intel.com>
> > > wrote:
> > > > On Fri, 2022-06-03 at 16:32 +0000, Souza, Jose wrote:
> > > > > On Fri, 2022-06-03 at 13:14 +0000, Hogander, Jouni wrote:
> > > > > > On Fri, 2022-06-03 at 15:43 +0300, Jani Nikula wrote:
> > > > > > > On Fri, 03 Jun 2022, Jouni Högander <
> > > > > > > jouni.hogander at intel.com>
> > > > > > > wrote:
> > > > > > > > Export headless sku bit (bit 13) from opregion->header-
> > > > > > > > >pcon as
> > > > > > > > an
> > > > > > > > interface to check if our device is headless
> > > > > > > > configuration.
> > > > > > > > 
> > > > > > > > Bspec: 53441
> > > > > > > > Signed-off-by: Jouni Högander <jouni.hogander at intel.com
> > > > > > > > >
> > > > > > > > ---
> > > > > > > >  drivers/gpu/drm/i915/display/intel_opregion.c | 12
> > > > > > > > ++++++++++++
> > > > > > > >  drivers/gpu/drm/i915/display/intel_opregion.h |  7
> > > > > > > > +++++++
> > > > > > > >  2 files changed, 19 insertions(+)
> > > > > > > > 
> > > > > > > > diff --git
> > > > > > > > a/drivers/gpu/drm/i915/display/intel_opregion.c
> > > > > > > > b/drivers/gpu/drm/i915/display/intel_opregion.c
> > > > > > > > index f31e8c3f8ce0..eab3f2e6b786 100644
> > > > > > > > --- a/drivers/gpu/drm/i915/display/intel_opregion.c
> > > > > > > > +++ b/drivers/gpu/drm/i915/display/intel_opregion.c
> > > > > > > > @@ -53,6 +53,8 @@
> > > > > > > >  #define MBOX_ASLE_EXTBIT(4)/* Mailbox #5 */
> > > > > > > >  #define MBOX_BACKLIGHTBIT(5)/* Mailbox #2
> > > > > > > > (valid from v3.x) */
> > > > > > > > 
> > > > > > > > +#define PCON_HEADLESS_SKUBIT(13)
> > > > > > > 
> > > > > > > Here we go again.
> > > > > > > 
> > > > > > > What does headless mean here? The spec does not say. Does
> > > > > > > it have
> > > > > > > display hardware? Apparently yes, since otherwise we
> > > > > > > wouldn't be
> > > > > > > here.
> > > > > > 
> > > > > > This is for hybrid setup with several display hw and the
> > > > > > panel wont
> > > > > > be
> > > > > > connected into device driven by i915 driver.
> > > > > > 
> > > > > > > We have INTEL_DISPLAY_ENABLED() which should do the right
> > > > > > > thing
> > > > > > > when
> > > > > > > you
> > > > > > > do have display hardware and have done output setup etc.
> > > > > > > but want
> > > > > > > to
> > > > > > > force them disconnected, i.e. you take the hardware over
> > > > > > > properly,
> > > > > > > but
> > > > > > > put it to sleep for power savings.
> > > > > > > 
> > > > > > > Maybe we should bolt this opregion check in that macro?
> > > > > > > 
> > > > > > > Maybe we need to use INTEL_DISPLAY_ENABLED() also to
> > > > > > > prevent
> > > > > > > polling.
> > > > > > 
> > > > > > Thank you for pointing this out. HAS_DISPLAY I already
> > > > > > notice and
> > > > > > it's
> > > > > > not suitable for what we want here. I think bolting this
> > > > > > check into
> > > > > > INTEL_DISPLAY_ENABLED as you suggested is enough. That will
> > > > > > prevent
> > > > > > waking up the hw into D0 state for polling.
> > > > > 
> > > > > A headless sku should not have any DDI ports enabled, much
> > > > > easier
> > > > > check for that.
> > > > 
> > > > Could you please clarify this a bit? What exactly you are
> > > > thinking
> > > > should be checked? Aren't DDI port also disabled when non-
> > > > headless
> > > > setup is in runtime suspend?
> > > 
> > > I also think "headless" and "DDI ports enabled" need
> > > clarification. They
> > > are overloaded terms.
> > 
> > In a properly setup headless sku, VBT should have all ports marked
> > as disabled.
> 
> This is what I mean with overloaded terms. Now we're at "properly
> setup
> headless sku", and we're none the wiser. ;)
> 
> Is the hardware there?
> 
> If not, okay, but why aren't the pipes fused then? Why a different
> flag
> in opregion?

I understand it as a "master" bit to disable having any display
connected to this hardware. To my understanding target use for
operegion headless bit is hybrid graphics setup.

> 
> If yes, has the GOP taken over the hardware and put it into power
> save?
> 
> BR,
> Jani.
> 
> 
> > intel_ddi_init() {
> > 	...
> > 
> > 	if (!init_dp && !init_hdmi) {
> > 		drm_dbg_kms(&dev_priv->drm,
> > 			    "VBT says port %c is not DVI/HDMI/DP
> > compatible, respect it\n",
> > 			    port_name(port));
> > 		return;
> > 	}
> > 
> > 
> > All DDI should return earlier in the above.
> > So you can use the number of enabled connectors to know if it is a
> > headless sku or not.
> > 
> > So you can skip the pooling in case there is no connectors.
> > 
> > > Seems to me the use case here could be the same as
> > > INTEL_DISPLAY_ENABLED(), and that could benefit from polling
> > > disable.
> > > 
> > > BR,
> > > Jani.
> > > 
> > > 
> > > >  
> > > > > > > I certainly would not want to add another mode that's
> > > > > > > separate
> > > > > > > from
> > > > > > > HAS_DISPLAY() and INTEL_DISPLAY_ENABLED().
> > > > > > 
> > > > > > No need for this. I think we can go with
> > > > > > INTEL_DISPLAY_ENABLED.
> > > > > > > > +
> > > > > > > >  struct opregion_header {
> > > > > > > >  u8 signature[16];
> > > > > > > >  u32 size;
> > > > > > > > @@ -1135,6 +1137,16 @@ struct edid
> > > > > > > > *intel_opregion_get_edid(struct
> > > > > > > > intel_connector *intel_connector)
> > > > > > > >  return new_edid;
> > > > > > > >  }
> > > > > > > > 
> > > > > > > > +bool intel_opregion_headless_sku(struct
> > > > > > > > drm_i915_private
> > > > > > > > *i915)
> > > > > > > > +{
> > > > > > > > +struct intel_opregion *opregion = &i915->opregion;
> > > > > > > > +
> > > > > > > > +if (!opregion->header)
> > > > > > > > +return false;
> > > > > > > > +
> > > > > > > > +return opregion->header->pcon & PCON_HEADLESS_SKU;
> > > > > > > 
> > > > > > > We should probably start checking for opregion version
> > > > > > > for this
> > > > > > > stuff
> > > > > > > too.
> > > > > > > 
> > > > > > 
> > > > > > Yes, I will do this change.
> > > > > > 
> > > > > > > BR,
> > > > > > > Jani.
> > > > > > > 
> > > > > > > > +}
> > > > > > > > +
> > > > > > > >  void intel_opregion_register(struct drm_i915_private
> > > > > > > > *i915)
> > > > > > > >  {
> > > > > > > >  struct intel_opregion *opregion = &i915->opregion;
> > > > > > > > diff --git
> > > > > > > > a/drivers/gpu/drm/i915/display/intel_opregion.h
> > > > > > > > b/drivers/gpu/drm/i915/display/intel_opregion.h
> > > > > > > > index 82cc0ba34af7..5ad96e1d8278 100644
> > > > > > > > --- a/drivers/gpu/drm/i915/display/intel_opregion.h
> > > > > > > > +++ b/drivers/gpu/drm/i915/display/intel_opregion.h
> > > > > > > > @@ -76,6 +76,8 @@ int
> > > > > > > > intel_opregion_notify_adapter(struct
> > > > > > > > drm_i915_private *dev_priv,
> > > > > > > >  int intel_opregion_get_panel_type(struct
> > > > > > > > drm_i915_private
> > > > > > > > *dev_priv);
> > > > > > > >  struct edid *intel_opregion_get_edid(struct
> > > > > > > > intel_connector
> > > > > > > > *connector);
> > > > > > > > 
> > > > > > > > +bool intel_opregion_headless_sku(struct
> > > > > > > > drm_i915_private
> > > > > > > > *i915);
> > > > > > > > +
> > > > > > > >  #else /* CONFIG_ACPI*/
> > > > > > > > 
> > > > > > > >  static inline int intel_opregion_setup(struct
> > > > > > > > drm_i915_private
> > > > > > > > *dev_priv)
> > > > > > > > @@ -127,6 +129,11 @@ intel_opregion_get_edid(struct
> > > > > > > > intel_connector
> > > > > > > > *connector)
> > > > > > > >  return NULL;
> > > > > > > >  }
> > > > > > > > 
> > > > > > > > +bool intel_opregion_headless_sku(struct
> > > > > > > > drm_i915_private
> > > > > > > > *i915)
> > > > > > > > +{
> > > > > > > > +return false;
> > > > > > > > +}
> > > > > > > > +
> > > > > > > >  #endif /* CONFIG_ACPI */
> > > > > > > > 
> > > > > > > >  #endif