2013/8/29 Jani Nikula <jani.nikula@xxxxxxxxx>:
> On Thu, 29 Aug 2013, Paulo Zanoni <przanoni@xxxxxxxxx> wrote:
>> Hi
>>
>> 2013/8/23 Jani Nikula <jani.nikula@xxxxxxxxx>:
>>> 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@xxxxxxxxx>
>>> ---
>>> 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.
I re-checked the code and now I see why we're both confused :). We
have 2 request_callbacks functions: the GBDA one and the SBCB one. If
you look at patch 2, you'll notice that at intel_opregion_setup we
request the GBDA callbacks. But if you look at the swsci function
you'll see that our requested_callbacks check is for the SBCB main
function. For some reason I was thinking we only did the "check and
store requested callbacks" for GBDA, so I suggested you to implement
the same thing (inside the swsci funcion), but for SBCB: with this, we
wouldn't need check at the call sites. But I see that in the current
code we call the "give me the GBDA callbacks" but never check them,
and we don't call the "give me the SBCB callbacks" but always check
them.
Also, on patch 2 when I was saying that we need the "-1" on the shift
and that some bits moved around, I was reviewing the SBCB code, not
the GBDA, we need to check both...
>
> BR,
> Jani.
>
>
>>
>> With or without any changes: Reviewed-by: Paulo Zanoni
>> <paulo.r.zanoni@xxxxxxxxx>
>>
>>
>>> +}
>>> +
>>> 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@xxxxxxxxxxxxxxxxxxxxx
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Paulo Zanoni
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
