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

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

On Thu, 29 Aug 2013, Paulo Zanoni <przanoni@gmail.com> wrote:
> Hi
>
> 2013/8/23 Jani Nikula <jani.nikula@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@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.

Right.

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

As I explained in my reply to your review on patch 2, my idea was to
filter out unsupported calls in swsci(), so we don't have to add checks
to all call sites. I also log the swsci_requested_callbacks value after
GBDA. We could add a bigger warn (as a separate patch) after the GBDA
call if some needed callback is not requested.

BR,
Jani.


>
> With or without any changes: Reviewed-by: Paulo Zanoni
> <paulo.r.zanoni@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
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx