From mboxrd@z Thu Jan 1 00:00:00 1970 From: Vandana Kannan Subject: Re: [PATCH 2/5] drm/i915: Parse EDID probed modes for DRRS support Date: Tue, 11 Feb 2014 12:02:30 +0530 Message-ID: <52F9C3FE.4070304@intel.com> References: <1387785153-5329-1-git-send-email-vandana.kannan@intel.com> <1387785153-5329-3-git-send-email-vandana.kannan@intel.com> <87mwio5egc.fsf@intel.com> <52E9C810.7000506@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by gabe.freedesktop.org (Postfix) with ESMTP id A9DEAFA8EF for ; Mon, 10 Feb 2014 22:32:34 -0800 (PST) In-Reply-To: <52E9C810.7000506@intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces@lists.freedesktop.org Errors-To: intel-gfx-bounces@lists.freedesktop.org To: Jani Nikula Cc: "intel-gfx@lists.freedesktop.org" List-Id: intel-gfx@lists.freedesktop.org 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 wrote: >>> From: Pradeep Bhat >>> >>> 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 >>> Signed-off-by: Vandana Kannan >>> --- >>> 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 "[Intel-gfx] 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@lists.freedesktop.org >>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx >> >