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

[Jani Nikula <jani.nikula@linux.intel.com>]

On Mon, 06 Jun 2022, "Hogander, Jouni" <jouni.hogander@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@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@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.

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
>

-- 
Jani Nikula, Intel Open Source Graphics Center