| Message ID | 20220603101411.3087789-2-jouni.hogander@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Disable connector polling for a headless sku | expand |
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_EXT BIT(4) /* Mailbox #5 */ > #define MBOX_BACKLIGHT BIT(5) /* Mailbox #2 (valid from v3.x) */ > > +#define PCON_HEADLESS_SKU BIT(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. 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. I certainly would not want to add another mode that's separate from HAS_DISPLAY() and 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. 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
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_EXT BIT(4) /* Mailbox #5 */ > > #define MBOX_BACKLIGHT BIT(5) /* Mailbox #2 > > (valid from v3.x) */ > > > > +#define PCON_HEADLESS_SKU BIT(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. > > 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
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_EXT BIT(4) /* Mailbox #5 */ > > > #define MBOX_BACKLIGHT BIT(5) /* Mailbox #2 > > > (valid from v3.x) */ > > > > > > +#define PCON_HEADLESS_SKU BIT(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. > > > > > 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 >
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 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
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 >
On Mon, 2022-06-06 at 11:16 +0300, Jani Nikula wrote: > 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. In a properly setup headless sku, VBT should have all ports marked as disabled. 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 > > >
On Mon, 06 Jun 2022, "Souza, Jose" <jose.souza@intel.com> wrote: > On Mon, 2022-06-06 at 11:16 +0300, Jani Nikula wrote: >> 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. > > 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? 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 >> > >> >
On Tue, 2022-06-07 at 10:36 +0300, Jani Nikula wrote: > On Mon, 06 Jun 2022, "Souza, Jose" <jose.souza@intel.com> wrote: > > On Mon, 2022-06-06 at 11:16 +0300, Jani Nikula wrote: > > > 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. > > > > 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
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.souza@intel.com> wrote: > > > On Mon, 2022-06-06 at 11:16 +0300, Jani Nikula wrote: > > > > 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. > > > > > > 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 >
On Mon, 2022-06-06 at 13:15 +0000, Souza, Jose wrote: > On Mon, 2022-06-06 at 11:16 +0300, Jani Nikula wrote: > > 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. > > In a properly setup headless sku, VBT should have all ports marked as > disabled. Should we take into account headless bit in opregion possibly conflicting with VBT contents? Now as you pointed out this I started to think intel_opregion_headless_sku check could be also in here as additional check: if (!init_dp && !init_hdmi || intel_opregion_headless_sku()) { This would prevent initializing any connector including setting them up for polling? > > 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
On Mon, 2022-06-06 at 13:15 +0000, Souza, Jose wrote: > On Mon, 2022-06-06 at 11:16 +0300, Jani Nikula wrote: > > 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. > > In a properly setup headless sku, VBT should have all ports marked as > disabled. > > 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. I went through this and to my understanding this polling doesn't happen in setup where all ports are marked as disabled in VBT. Connector is not initialized at all -> polling is not enabled when runtime suspend happens. BR, Jouni Högander
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_EXT BIT(4) /* Mailbox #5 */ #define MBOX_BACKLIGHT BIT(5) /* Mailbox #2 (valid from v3.x) */ +#define PCON_HEADLESS_SKU BIT(13) + 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; +} + 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
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(+)