From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Vetter Subject: Re: [PATCH] drm/i915: Hook PSR functionality Date: Mon, 8 Jul 2013 21:46:57 +0200 Message-ID: <20130708194657.GF18285@phenom.ffwll.local> References: <1372726331-31617-1-git-send-email-rodrigo.vivi@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail-ee0-f51.google.com (mail-ee0-f51.google.com [74.125.83.51]) by gabe.freedesktop.org (Postfix) with ESMTP id 13948E5DB0 for ; Mon, 8 Jul 2013 12:46:57 -0700 (PDT) Received: by mail-ee0-f51.google.com with SMTP id e52so3116769eek.10 for ; Mon, 08 Jul 2013 12:46:57 -0700 (PDT) Content-Disposition: inline In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org Errors-To: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org To: Paulo Zanoni Cc: intel-gfx@lists.freedesktop.org, Paulo Zanoni List-Id: intel-gfx@lists.freedesktop.org On Mon, Jul 08, 2013 at 10:13:30AM -0300, Paulo Zanoni wrote: > 2013/7/1 Rodrigo Vivi : > > PSR must be enabled after transcoder and port are running. > > And it is only available for HSW. > > > > v2: move enable/disable to intel_ddi > > v3: The spec suggests PSR should be disabled even before backlight (by pzanoni) > > v4: also disabling and enabling whenever panel is disabled/enabled. > > v5: make it last patch to avoid breaking whenever bisecting. So calling for > > update and force exit came to this patch along with enable/disable calls. > > v6: Remove unused and unecessary psr_enable/disable calls, as notice by Paulo. > > > > CC: Paulo Zanoni > > Signed-off-by: Rodrigo Vivi > > --- > > drivers/gpu/drm/i915/i915_gem.c | 2 ++ > > drivers/gpu/drm/i915/intel_ddi.c | 2 ++ > > drivers/gpu/drm/i915/intel_display.c | 1 + > > 3 files changed, 5 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > > index c68b90f..f2e135b 100644 > > --- a/drivers/gpu/drm/i915/i915_gem.c > > +++ b/drivers/gpu/drm/i915/i915_gem.c > > @@ -3786,6 +3786,8 @@ i915_gem_busy_ioctl(struct drm_device *dev, void *data, > > goto unlock; > > } > > > > + intel_edp_psr_force_exit(dev); > > Shouldn't we enter/leave PSR in exactly the same cases as we > enter/leave FBC? This function doesn't seem to touch FBC. I also see, > for example, intel_unpin_work_fn calling intel_update_fbc, but at that > point we don't update PSR. Also intel_enable_primary and > intel_disable_primary also call intel_update_fbc. If we conclude that > PSR and FBC need to be disabled/enabled at exactly the same places, > can we do some rework to unify this? I think there's some room for fbc and psr handling code to differ, since fbc only works on the primary plane, whereas psr works on the entire display (so also needs to update when cursor/sprites change or gamma table changes or any other disaplay attribute). But I agree with you that we should share calls to psr/fbc placed in common gem/modeset code as much as possible. -Daniel > > > > + > > /* Count all active objects as busy, even if they are currently not used > > * by the gpu. Users of this interface expect objects to eventually > > * become non-busy without any further actions, therefore emit any > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c > > index 324211a..4211925 100644 > > --- a/drivers/gpu/drm/i915/intel_ddi.c > > +++ b/drivers/gpu/drm/i915/intel_ddi.c > > @@ -1117,6 +1117,7 @@ static void intel_enable_ddi(struct intel_encoder *intel_encoder) > > intel_dp_stop_link_train(intel_dp); > > > > ironlake_edp_backlight_on(intel_dp); > > + intel_edp_psr_enable(intel_dp); > > } > > > > if (intel_crtc->eld_vld && type != INTEL_OUTPUT_EDP) { > > @@ -1147,6 +1148,7 @@ static void intel_disable_ddi(struct intel_encoder *intel_encoder) > > if (type == INTEL_OUTPUT_EDP) { > > struct intel_dp *intel_dp = enc_to_intel_dp(encoder); > > > > + intel_edp_psr_disable(intel_dp); > > ironlake_edp_backlight_off(intel_dp); > > } > > } > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > index 6fafa43..5bbfed0 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -2236,6 +2236,7 @@ intel_pipe_set_base(struct drm_crtc *crtc, int x, int y, > > } > > > > intel_update_fbc(dev); > > + intel_edp_psr_update(dev); > > mutex_unlock(&dev->struct_mutex); > > > > intel_crtc_update_sarea_pos(crtc, x, y); > > -- > > 1.8.1.4 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > -- > Paulo Zanoni > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch