On Jan-30-2014 9:03 AM, Vandana Kannan wrote:
> On Jan-22-2014 7:03 PM, Jani Nikula wrote:
>> On Mon, 23 Dec 2013, Vandana Kannan <vandana.kannan@xxxxxxxxx> wrote:
>>> From: Pradeep Bhat <pradeep.bhat@xxxxxxxxx>
>>>
>>> This patch and finds out the lowest refresh rate supported for the resolution
>>> same as the fixed_mode, based on the implementaion find_panel_downclock.
>>> It also checks the VBT fields to see if panel supports seamless DRRS or not.
>>> Based on above data it marks whether eDP panel supports seamless DRRS or not.
>>> This information is needed for supporting seamless DRRS switch for certain
>>> power saving usecases. This patch is tested by enabling the DRM logs and
>>> user should see whether Seamless DRRS is supported or not.
>>
>> This patch (and therefore the later patches) no longer apply to
>> drm-intel-nightly. It might affect my review a bit, but here goes
>> anyway.
>>
> I will rebase and resend the patch.
>>>
>>> v2: Daniel's review comments
>>> Modified downclock deduction based on intel_find_panel_downclock
>>>
>>> v3: Chris's review comments
>>> Moved edp_downclock_avail and edp_downclock to intel_panel
>>>
>>> v4: Jani's review comments.
>>> Changed name of the enum edp_panel_type to drrs_support type.
>>> Change is_drrs_supported to drrs_support of type enum drrs_support_type.
>>>
>>> Signed-off-by: Pradeep Bhat <pradeep.bhat@xxxxxxxxx>
>>> Signed-off-by: Vandana Kannan <vandana.kannan@xxxxxxxxx>
>>> ---
>>> drivers/gpu/drm/i915/intel_dp.c | 45 ++++++++++++++++++++++++++++++++++++++
>>> drivers/gpu/drm/i915/intel_drv.h | 30 +++++++++++++++++++++++++
>>> 2 files changed, 75 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>>> index 8f17f8f..079b53f 100644
>>> --- a/drivers/gpu/drm/i915/intel_dp.c
>>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>>> @@ -3522,6 +3522,46 @@ intel_dp_init_panel_power_sequencer_registers(struct drm_device *dev,
>>> I915_READ(pp_div_reg));
>>> }
>>>
>>> +static void
>>> +intel_dp_drrs_initialize(struct intel_digital_port *intel_dig_port,
>>> + struct intel_connector *intel_connector,
>>> + struct drm_display_mode *fixed_mode) {
>>
>> I'll explain later why I think you should change the signature of the
>> function.
>>
>>> + struct drm_connector *connector = &intel_connector->base;
>>> + struct intel_dp *intel_dp = &intel_dig_port->dp;
>>> + struct drm_device *dev = intel_dig_port->base.base.dev;
>>> + struct drm_i915_private *dev_priv = dev->dev_private;
>>> +
>>> + /**
>>> + * Check if PSR is supported by panel and enabled
>>> + * if so then DRRS is reported as not supported for Haswell.
>>> + */
>>> + if (INTEL_INFO(dev)->gen < 8 && intel_edp_is_psr_enabled(dev)) {
>>> + DRM_INFO("eDP panel has PSR enabled. Cannot support DRRS\n");
>>> + return;
>>> + }
>>> +
>>> + /* First check if DRRS is enabled from VBT struct */
>>> + if (!dev_priv->vbt.drrs_enabled) {
>>> + DRM_INFO("VBT doesn't support DRRS\n");
>>> + return;
>>> + }
>>> +
>>> + intel_connector->panel.downclock_mode = intel_find_panel_downclock(dev,
>>> + fixed_mode, connector);
>>> +
>>> + if (intel_connector->panel.downclock_mode != NULL &&
>>> + dev_priv->vbt.drrs_mode == SEAMLESS_DRRS_SUPPORT) {
>>> + intel_connector->panel.edp_downclock_avail = true;
>>
>> If you rearranged the code a bit, you could make the
>> panel.downclock_mode != NULL mean the same as
>> edp_downclock_avail. I.e. if you have the downclock_mode there, it's
>> available.
>>
> This was done to be in sync with lvds_downclock implementation based on
> previous review comments.
>>> + intel_connector->panel.edp_downclock =
>>> + intel_connector->panel.downclock_mode->clock;
>>
>> I don't understand why you need two copies of the clock.
>>
>> In general, we should try and avoid adding extra state and copies of
>> information for stuff that we can readily derive from other information.
>>
>>> +
>>> + intel_dp->drrs_state.drrs_support = dev_priv->vbt.drrs_mode;
>>
>> Again. I can't see intel_dp->drrs_state.drrs_support ever needing to be
>> different from dev_priv->vbt.drrs_mode. So why the copy?
>>
> This was done to make things more readable.
>>> +
>>> + intel_dp->drrs_state.drrs_refresh_rate_type = DRRS_HIGH_RR;
>>> + DRM_INFO("SEAMLESS DRRS supported for eDP panel.\n");
>>> + }
>>> +}
>>> +
>>> static bool intel_edp_init_connector(struct intel_dp *intel_dp,
>>> struct intel_connector *intel_connector)
>>> {
>>> @@ -3535,6 +3575,8 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
>>> struct drm_display_mode *scan;
>>> struct edid *edid;
>>>
>>> + intel_dp->drrs_state.drrs_support = DRRS_NOT_SUPPORTED;
>>> +
>>> if (!is_edp(intel_dp))
>>> return true;
>>>
>>> @@ -3579,6 +3621,9 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
>>> list_for_each_entry(scan, &connector->probed_modes, head) {
>>> if ((scan->type & DRM_MODE_TYPE_PREFERRED)) {
>>> fixed_mode = drm_mode_duplicate(dev, scan);
>>> + if (INTEL_INFO(dev)->gen >= 5)
>>> + intel_dp_drrs_initialize(intel_dig_port,
>>> + intel_connector, fixed_mode);
>>
>> Is there any reason not to do this at the top level after checking for
>> the VBT mode?
>>
> This was done as fixed_mode was required.
>
>> Also, we have a separate function for initializing the panel struct, so
>> I think you should make intel_dp_drrs_initialize() return the downclock
>> mode or NULL, and pass that to intel_panel_init() instead of
>> initializing the panel struct directly within the function.
>>
> I will make this change.
I have submitted a patch " drm/i915: Initialize downclock
mode in panel init" to modify intel_panel_init() and all its callers.
>>> break;
>>> }
>>> }
>>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>>> index e903432..d208bf5 100644
>>> --- a/drivers/gpu/drm/i915/intel_drv.h
>>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>>> @@ -168,6 +168,9 @@ struct intel_panel {
>>> bool active_low_pwm;
>>> struct backlight_device *device;
>>> } backlight;
>>> +
>>> + bool edp_downclock_avail;
>>> + int edp_downclock;
>>
>> As I said, I think you can get rid of both of these.
>>
> As mentioned above, this was done to be in sync with lvds_downclock
> implementation based on previous review comments.
>>> };
>>>
>>> struct intel_connector {
>>> @@ -462,6 +465,32 @@ struct intel_hdmi {
>>>
>>> #define DP_MAX_DOWNSTREAM_PORTS 0x10
>>>
>>> +/**
>>> + * This enum is used to indicate the DRRS support type.
>>> + */
>>> +enum drrs_support_type {
>>> + DRRS_NOT_SUPPORTED = -1,
>>> + STATIC_DRRS_SUPPORT = 0, /* 1:1 mapping with VBT */
>>> + SEAMLESS_DRRS_SUPPORT = 2 /* 1:1 mapping with VBT */ };
>>
>> I don't see any value in having 1:1 mapping with VBT. Not even in having
>> 1:1 mapping between struct intel_vbt_data and the actual VBT. It's
>> supposed to be parsed data.
>>
>> Instead, I do see value in making DRRS_NOT_SUPPORTED == 0 as the logical
>> thing to do.
>>
> Ok. I will make necessary changes..
>>> +/**
>>> + * HIGH_RR is the highest eDP panel refresh rate read from EDID
>>> + * LOW_RR is the lowest eDP panel refresh rate found from EDID
>>> + * parsing for same resolution.
>>> + */
>>> +enum edp_drrs_refresh_rate_type {
>>> + DRRS_HIGH_RR,
>>> + DRRS_LOW_RR,
>>> + DRRS_MAX_RR, /* RR count */
>>> +};
>>> +/**
>>> + * The drrs_info struct will represent the DRRS feature for eDP
>>> + * panel.
>>> + */
>>
>> This comment does not add any value.
>>
> Ok.
>>> +struct drrs_info {
>>> + enum drrs_support_type drrs_support;
>>> + enum edp_drrs_refresh_rate_type drrs_refresh_rate_type;
>>
>> Because this will be accessed through intel_dp->drrs_state, there's no
>> need to duplicate "drrs" in the field names here. It will be obvious
>> from the context.
>>
> Ok.
>>> +};
>>> +
>>> struct intel_dp {
>>> uint32_t output_reg;
>>> uint32_t aux_ch_ctl_reg;
>>> @@ -487,6 +516,7 @@ struct intel_dp {
>>> bool want_panel_vdd;
>>> bool psr_setup_done;
>>> struct intel_connector *attached_connector;
>>> + struct drrs_info drrs_state;
>>> };
>>>
>>> struct intel_digital_port {
>>> --
>>> 1.7.9.5
>>>
>>> _______________________________________________
>>> Intel-gfx mailing list
>>> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
>>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>>
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
