On Mon, 09 Dec 2013, Vandana Kannan <vandana.kannan@xxxxxxxxx> wrote:
> If one mode of a internal panel has more than one refresh rate, then a reduced
> clock is found for the LFP (LVDS/eDP). This enables switching between low
> and high frequency dynamically. Moving downclock calculation to intel_panel
> so that it is common for LVDS and eDP.
>
> Signed-off-by: Vandana Kannan <vandana.kannan@xxxxxxxxx>
> Signed-off-by: Pradeep Bhat <pradeep.bhat@xxxxxxxxx>
> ---
> drivers/gpu/drm/i915/intel_drv.h | 6 +++-
> drivers/gpu/drm/i915/intel_lvds.c | 69 +++++++++---------------------------
> drivers/gpu/drm/i915/intel_panel.c | 56 +++++++++++++++++++++++++++++
> 3 files changed, 77 insertions(+), 54 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 5dea389..17e8964 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -155,6 +155,7 @@ struct intel_encoder {
>
> struct intel_panel {
> struct drm_display_mode *fixed_mode;
> + struct drm_display_mode *downclock_mode;
> int fitting_mode;
>
> /* backlight */
> @@ -823,7 +824,10 @@ void intel_panel_disable_backlight(struct intel_connector *connector);
> void intel_panel_destroy_backlight(struct drm_connector *connector);
> void intel_panel_init_backlight_funcs(struct drm_device *dev);
> enum drm_connector_status intel_panel_detect(struct drm_device *dev);
> -
> +extern bool intel_find_panel_downclock(struct drm_device *dev,
> + struct intel_panel *panel,
> + struct drm_display_mode *fixed_mode,
> + struct drm_connector *connector);
>
> /* intel_pm.c */
> void intel_init_clock_gating(struct drm_device *dev);
> diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
> index 3deb58e..a589814 100644
> --- a/drivers/gpu/drm/i915/intel_lvds.c
> +++ b/drivers/gpu/drm/i915/intel_lvds.c
> @@ -745,57 +745,6 @@ static const struct dmi_system_id intel_no_lvds[] = {
> { } /* terminating entry */
> };
>
> -/**
> - * intel_find_lvds_downclock - find the reduced downclock for LVDS in EDID
> - * @dev: drm device
> - * @connector: LVDS connector
> - *
> - * Find the reduced downclock for LVDS in EDID.
> - */
> -static void intel_find_lvds_downclock(struct drm_device *dev,
> - struct drm_display_mode *fixed_mode,
> - struct drm_connector *connector)
> -{
> - struct drm_i915_private *dev_priv = dev->dev_private;
> - struct drm_display_mode *scan;
> - int temp_downclock;
> -
> - temp_downclock = fixed_mode->clock;
> - list_for_each_entry(scan, &connector->probed_modes, head) {
> - /*
> - * If one mode has the same resolution with the fixed_panel
> - * mode while they have the different refresh rate, it means
> - * that the reduced downclock is found for the LVDS. In such
> - * case we can set the different FPx0/1 to dynamically select
> - * between low and high frequency.
> - */
> - if (scan->hdisplay == fixed_mode->hdisplay &&
> - scan->hsync_start == fixed_mode->hsync_start &&
> - scan->hsync_end == fixed_mode->hsync_end &&
> - scan->htotal == fixed_mode->htotal &&
> - scan->vdisplay == fixed_mode->vdisplay &&
> - scan->vsync_start == fixed_mode->vsync_start &&
> - scan->vsync_end == fixed_mode->vsync_end &&
> - scan->vtotal == fixed_mode->vtotal) {
> - if (scan->clock < temp_downclock) {
> - /*
> - * The downclock is already found. But we
> - * expect to find the lower downclock.
> - */
> - temp_downclock = scan->clock;
> - }
> - }
> - }
> - if (temp_downclock < fixed_mode->clock && i915_lvds_downclock) {
> - /* We found the downclock for LVDS. */
> - dev_priv->lvds_downclock_avail = 1;
> - dev_priv->lvds_downclock = temp_downclock;
> - DRM_DEBUG_KMS("LVDS downclock is found in EDID. "
> - "Normal clock %dKhz, downclock %dKhz\n",
> - fixed_mode->clock, temp_downclock);
> - }
> -}
> -
> /*
> * Enumerate the child dev array parsed from VBT to check whether
> * the LVDS is present.
> @@ -1073,8 +1022,22 @@ void intel_lvds_init(struct drm_device *dev)
>
> fixed_mode = drm_mode_duplicate(dev, scan);
> if (fixed_mode) {
> - intel_find_lvds_downclock(dev, fixed_mode,
> - connector);
> + dev_priv->lvds_downclock_avail =
> + intel_find_panel_downclock(dev,
> + &intel_connector->panel,
> + fixed_mode, connector);
> + if (dev_priv->lvds_downclock_avail &&
> + i915_lvds_downclock) {
> + /* We found the downclock for LVDS. */
> + dev_priv->lvds_downclock =
> + intel_connector->panel.
> + downclock_mode->clock;
> + DRM_DEBUG_KMS("LVDS downclock is found"
> + " in EDID. Normal clock %dKhz, "
> + "downclock %dKhz\n",
> + fixed_mode->clock,
> + dev_priv->lvds_downclock);
> + }
> goto out;
> }
> }
> diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
> index e480cf4..f49a136 100644
> --- a/drivers/gpu/drm/i915/intel_panel.c
> +++ b/drivers/gpu/drm/i915/intel_panel.c
> @@ -1104,6 +1104,62 @@ void intel_panel_destroy_backlight(struct drm_connector *connector)
> intel_backlight_device_unregister(intel_connector);
> }
>
> +/**
> + * intel_find_panel_downclock - find the reduced downclock for LVDS in EDID
> + * @dev: drm device
> + * @panel: intel panel
> + * @fixed_mode : panel native mode
> + * @connector: LVDS/eDP connector
> + *
> + * Return downclock_avail
> + * Find the reduced downclock for LVDS/eDP in EDID.
> + */
> +bool intel_find_panel_downclock(struct drm_device *dev,
> + struct intel_panel *panel,
> + struct drm_display_mode *fixed_mode,
> + struct drm_connector *connector)
I think I'd make this
struct drm_display_mode *
intel_panel_find_downclock(struct drm_connector *connector,
struct drm_display_mode *fixed_mode)
which would return a drm_mode_duplicate'd downclocked mode if one was
found, NULL otherwise. Then whoever calls the function would set
panel->downclock_mode explicitly instead of it happening as a side
effect here. (A "find" function that sets stuff is surprising! I know it
was a little like this already, but let's be a little more careful with
non-static functions.)
> +{
> + struct drm_display_mode *scan, *tmp_mode;
> + int temp_downclock;
> + bool ret = false;
> +
> + temp_downclock = fixed_mode->clock;
> + tmp_mode = NULL;
> + panel->downclock_mode = NULL;
> +
> + list_for_each_entry(scan, &connector->probed_modes, head) {
> + /*
> + * If one mode has the same resolution with the fixed_panel
> + * mode while they have the different refresh rate, it means
> + * that the reduced downclock is found. In such
> + * case we can set the different FPx0/1 to dynamically select
> + * between low and high frequency.
> + */
> + if (scan->hdisplay == fixed_mode->hdisplay &&
> + scan->hsync_start == fixed_mode->hsync_start &&
> + scan->hsync_end == fixed_mode->hsync_end &&
> + scan->vdisplay == fixed_mode->vdisplay &&
> + scan->vsync_start == fixed_mode->vsync_start &&
> + scan->vsync_end == fixed_mode->vsync_end) {
You drop htotal and vtotal from the check. Is this intentional? It
*must* be a separate patch with a commit message that justifies the
removal.
> + if (scan->clock < temp_downclock) {
> + /*
> + * The downclock is already found. But we
> + * expect to find the lower downclock.
> + */
> + temp_downclock = scan->clock;
> + tmp_mode = scan;
> + }
> + }
> + }
> +
> + if (temp_downclock < fixed_mode->clock) {
> + panel->downclock_mode = drm_mode_duplicate(dev, tmp_mode);
You need to drm_mode_destroy() the downclock_mode in intel_panel_fini().
BR,
Jani.
> + ret = true;
> + }
> +
> + return ret;
> +}
> +
> /* Set up chip specific backlight functions */
> void intel_panel_init_backlight_funcs(struct drm_device *dev)
> {
> --
> 1.7.9.5
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
