On Tue, 2022-06-07 at 13:05 +0000, Hogander, Jouni wrote:
> On Tue, 2022-06-07 at 10:36 +0300, Jani Nikula wrote:
> > On Mon, 06 Jun 2022, "Souza, Jose" <jose.so...@intel.com> wrote:
> > > On Mon, 2022-06-06 at 11:16 +0300, Jani Nikula wrote:
> > > > On Mon, 06 Jun 2022, "Hogander, Jouni" <jouni.hogan...@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.hogan...@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.hogan...@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?
All the display hardware it there but none of the DDI ports are connected to
physical(DP, HDMI, TC) ports.
Display can still be used with Wireless Display in Windows and maybe in future
with Linux!
> >
> > 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?
This was not happening in a notebook design that I fixed some issues, i915 it
the one that needs to do that.
> >
> > 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
>
