On Wed, 26 Mar 2014, Jani Nikula <jani.nik...@linux.intel.com> wrote:
> This and the following patches need to be rebased on top of current
> -nightly.
>
> On Fri, 07 Mar 2014, Vandana Kannan <vandana.kan...@intel.com> wrote:
>> From: Pradeep Bhat <pradeep.b...@intel.com>
>>
>> This patch and finds out the lowest refresh rate supported for the resolution
>> same as the fixed_mode.
>> 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.
>>
>> 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.
>>
>> v5: Incorporated Jani's review comments
>> Modify intel_dp_drrs_initialize to return downclock mode. Support for Gen7
>> and above.
>>
>> v6: Incorporated Chris's review comments.
>> Changed initialize to init in intel_drrs_initialize
>>
>> Signed-off-by: Pradeep Bhat <pradeep.b...@intel.com>
>> Signed-off-by: Vandana Kannan <vandana.kan...@intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_dp.c  |   54 
>> +++++++++++++++++++++++++++++++++++++-
>>  drivers/gpu/drm/i915/intel_drv.h |   20 ++++++++++++++
>>  2 files changed, 73 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_dp.c 
>> b/drivers/gpu/drm/i915/intel_dp.c
>> index 88cc9d3..39365bf 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -3666,6 +3666,50 @@ intel_dp_init_panel_power_sequencer_registers(struct 
>> drm_device *dev,
>>                    I915_READ(pp_div_reg));
>>  }
>>  
>> +static struct drm_display_mode *
>> +intel_dp_drrs_init(struct intel_digital_port *intel_dig_port,
>> +                    struct intel_connector *intel_connector,
>> +                    struct drm_display_mode *fixed_mode)
>> +{
>> +    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;
>> +    struct drm_display_mode *downclock_mode = NULL;
>> +
>> +    /**
>> +     * 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 downclock_mode;
>
> Just return NULL explicitly if that's the intention.
>
>> +    }
>> +
>> +    /* First check if DRRS is enabled from VBT struct */
>> +    if (dev_priv->vbt.drrs_type == DRRS_NOT_SUPPORTED) {
>> +            DRM_INFO("VBT doesn't support DRRS\n");
>> +            return downclock_mode;
>
> Same here.
>
>> +    }
>> +
>> +    downclock_mode = intel_find_panel_downclock
>> +                                    (dev, fixed_mode, connector);
>> +
>> +    if (downclock_mode != NULL &&
>> +            dev_priv->vbt.drrs_type == SEAMLESS_DRRS_SUPPORT) {
>> +            intel_connector->panel.edp_downclock_avail = true;
>> +            intel_connector->panel.edp_downclock =
>> +                    downclock_mode->clock;
>
> Why do you need a copy of downclock_mode->clock in
> intel_connector->panel.edp_downclock? You can always get that through
> intel_connector->panel.downclock_mode->clock. Single point of truth.

Also, what does intel_connector->panel.edp_downclock_avail indicate that
can't be derived from downclock_mode != NULL && dev_priv->vbt.drrs_type
== SEAMLESS_DRRS_SUPPORT?

>
>> +
>> +            intel_dp->drrs_state.type = dev_priv->vbt.drrs_type;
>> +
>> +            intel_dp->drrs_state.refresh_rate_type = DRRS_HIGH_RR;
>> +            DRM_INFO("seamless DRRS supported for eDP panel.\n");
>> +    }
>> +
>> +    return downclock_mode;
>> +}
>> +
>>  static bool intel_edp_init_connector(struct intel_dp *intel_dp,
>>                                   struct intel_connector *intel_connector,
>>                                   struct edp_power_seq *power_seq)
>> @@ -3675,10 +3719,13 @@ static bool intel_edp_init_connector(struct intel_dp 
>> *intel_dp,
>>      struct drm_device *dev = intel_dig_port->base.base.dev;
>>      struct drm_i915_private *dev_priv = dev->dev_private;
>>      struct drm_display_mode *fixed_mode = NULL;
>> +    struct drm_display_mode *downclock_mode = NULL;
>>      bool has_dpcd;
>>      struct drm_display_mode *scan;
>>      struct edid *edid;
>>  
>> +    intel_dp->drrs_state.type = DRRS_NOT_SUPPORTED;
>> +
>>      if (!is_edp(intel_dp))
>>              return true;
>>  
>> @@ -3720,6 +3767,11 @@ 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 > 6)
>
> It seems to me this condition should be inside intel_dp_drrs_init().
>
>> +                            downclock_mode =
>> +                                    intel_dp_drrs_init(
>> +                                            intel_dig_port,
>> +                                            intel_connector, fixed_mode);
>>                      break;
>>              }
>>      }
>> @@ -3732,7 +3784,7 @@ static bool intel_edp_init_connector(struct intel_dp 
>> *intel_dp,
>>                      fixed_mode->type |= DRM_MODE_TYPE_PREFERRED;
>>      }
>>  
>> -    intel_panel_init(&intel_connector->panel, fixed_mode, NULL);
>> +    intel_panel_init(&intel_connector->panel, fixed_mode, downclock_mode);
>>      intel_panel_setup_backlight(connector);
>>  
>>      return true;
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h 
>> b/drivers/gpu/drm/i915/intel_drv.h
>> index 6aa549a..c41c735 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;
>>  };
>>  
>>  struct intel_connector {
>> @@ -464,6 +467,22 @@ struct intel_hdmi {
>>  
>>  #define DP_MAX_DOWNSTREAM_PORTS             0x10
>>  
>> +/**
>> + * 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 */
>> +};
>> +
>> +struct drrs_info {
>> +    enum drrs_support_type type;
>> +    enum edp_drrs_refresh_rate_type refresh_rate_type;
>> +};
>> +
>>  struct intel_dp {
>>      uint32_t output_reg;
>>      uint32_t aux_ch_ctl_reg;
>> @@ -503,6 +522,7 @@ struct intel_dp {
>>                                   bool has_aux_irq,
>>                                   int send_bytes,
>>                                   uint32_t aux_clock_divider);
>> +    struct drrs_info drrs_state;
>
> Any reason this isn't an unnamed struct here? And if you need the name,
> why is it different from the field name? drrs_info vs. drrs_state.
>
>>  };
>>  
>>  struct intel_digital_port {
>> -- 
>> 1.7.9.5
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> -- 
> Jani Nikula, Intel Open Source Technology Center

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Reply via email to