[Intel-gfx] [PATCH 3/6] drm/i915: add opregion function to notify bios of encoder enable/disable

[Paulo Zanoni]

Hi

2013/8/23 Jani Nikula <jani.nikula at intel.com>:
> The bios interface seems messy, and it's hard to tell what the bios
> really wants. At first, only add support for DDI based machines (hsw+),
> and see how it turns out.
>
> The spec says to notify prior to power down and after power up. It is
> unclear whether it makes a difference.
>
> v2:
>  - squash notification function and callers patches together (Daniel)
>  - move callers to haswell_crtc_{enable,disable} (Daniel)
>  - rename notification function (Chris)
>
> v3:
>  - separate notification function and callers again, as it's not clear
>    whether the display power state notification is the right thing to do
>    after all
>
> Signed-off-by: Jani Nikula <jani.nikula at intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h       |    8 +++++
>  drivers/gpu/drm/i915/intel_opregion.c |   52 +++++++++++++++++++++++++++++++++
>  2 files changed, 60 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index adc2f46..1703029 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2171,15 +2171,23 @@ static inline bool intel_gmbus_is_forced_bit(struct i2c_adapter *adapter)
>  extern void intel_i2c_reset(struct drm_device *dev);
>
>  /* intel_opregion.c */
> +struct intel_encoder;
>  extern int intel_opregion_setup(struct drm_device *dev);
>  #ifdef CONFIG_ACPI
>  extern void intel_opregion_init(struct drm_device *dev);
>  extern void intel_opregion_fini(struct drm_device *dev);
>  extern void intel_opregion_asle_intr(struct drm_device *dev);
> +extern int intel_opregion_notify_encoder(struct intel_encoder *intel_encoder,
> +                                        bool enable);
>  #else
>  static inline void intel_opregion_init(struct drm_device *dev) { return; }
>  static inline void intel_opregion_fini(struct drm_device *dev) { return; }
>  static inline void intel_opregion_asle_intr(struct drm_device *dev) { return; }
> +static inline int
> +intel_opregion_notify_encoder(struct intel_encoder *intel_encoder, bool enable)
> +{
> +       return 0;
> +}
>  #endif
>
>  /* intel_acpi.c */
> diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c
> index a3558ae..72a4af6 100644
> --- a/drivers/gpu/drm/i915/intel_opregion.c
> +++ b/drivers/gpu/drm/i915/intel_opregion.c
> @@ -273,6 +273,58 @@ static int swsci(struct drm_device *dev, u32 function, u32 parm, u32 *parm_out)
>  #undef C
>  }
>
> +#define DISPLAY_TYPE_CRT                       0
> +#define DISPLAY_TYPE_TV                                1
> +#define DISPLAY_TYPE_EXTERNAL_FLAT_PANEL       2
> +#define DISPLAY_TYPE_INTERNAL_FLAT_PANEL       3
> +
> +int intel_opregion_notify_encoder(struct intel_encoder *intel_encoder,
> +                                 bool enable)
> +{
> +       struct drm_device *dev = intel_encoder->base.dev;
> +       u32 parm = 0;
> +       u32 type = 0;
> +       u32 port;
> +
> +       /* don't care about old stuff for now */
> +       if (!HAS_DDI(dev))
> +               return 0;
> +
> +       port = intel_ddi_get_encoder_port(intel_encoder);
> +       if (port == PORT_E) {
> +               port = 0;
> +       } else {
> +               parm |= 1 << port;
> +               port++;
> +       }
> +
> +       if (!enable)
> +               parm |= 4 << 8;
> +
> +       switch (intel_encoder->type) {
> +       case INTEL_OUTPUT_ANALOG:
> +               type = DISPLAY_TYPE_CRT;
> +               break;
> +       case INTEL_OUTPUT_UNKNOWN:
> +       case INTEL_OUTPUT_DISPLAYPORT:
> +       case INTEL_OUTPUT_HDMI:
> +               type = DISPLAY_TYPE_EXTERNAL_FLAT_PANEL;
> +               break;
> +       case INTEL_OUTPUT_LVDS:

We don't have LVDS on DDI platforms.


> +       case INTEL_OUTPUT_EDP:
> +               type = DISPLAY_TYPE_INTERNAL_FLAT_PANEL;
> +               break;
> +       default:
> +               DRM_DEBUG_DRIVER("unsupported intel_encoder type %d\n",
> +                                intel_encoder->type);
> +               return -EINVAL;
> +       }
> +
> +       parm |= type << (16 + port * 3);
> +
> +       return swsci(dev, SWSCI_SBCB_DISPLAY_POWER_STATE, parm, NULL);

In patch 2, on the initialization code you call the GBDA request
callbacks function to see which ones will work. Shouldn't you also add
code to call the SBCB request callbacks function and see if
DISPLAY_POWER_STATE is really supported? I know that
DISPLAY_POWER_STATE is supposed to be mandatory, but just checking
won't hurt us. We could maybe even add a code to WARN in case one of
the mandatory callbacks is not available :) This suggestion could be
in a separate patch.

With or without any changes: Reviewed-by: Paulo Zanoni
<paulo.r.zanoni at intel.com>


> +}
> +
>  static u32 asle_set_backlight(struct drm_device *dev, u32 bclp)
>  {
>         struct drm_i915_private *dev_priv = dev->dev_private;
> --
> 1.7.9.5
>



-- 
Paulo Zanoni