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

[Paulo Zanoni]

2013/8/29 Paulo Zanoni <przanoni at gmail.com>:
> 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);

This should be a WARN.


>> +               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



-- 
Paulo Zanoni