From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Vetter Subject: Re: [PATCH 10/19] drm/i915: check pipe power domain when reading its hw state Date: Wed, 5 Mar 2014 11:24:55 +0100 Message-ID: <20140305102455.GS17001@phenom.ffwll.local> References: <1392674540-10915-1-git-send-email-imre.deak@intel.com> <1392674540-10915-11-git-send-email-imre.deak@intel.com> <20140220113725.4e01d78b@jbarnes-desktop> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail-ea0-f181.google.com (mail-ea0-f181.google.com [209.85.215.181]) by gabe.freedesktop.org (Postfix) with ESMTP id 3D87AFA4A0 for ; Wed, 5 Mar 2014 02:25:00 -0800 (PST) Received: by mail-ea0-f181.google.com with SMTP id k10so923438eaj.12 for ; Wed, 05 Mar 2014 02:24:59 -0800 (PST) Content-Disposition: inline In-Reply-To: <20140220113725.4e01d78b@jbarnes-desktop> 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: Jesse Barnes Cc: intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org On Thu, Feb 20, 2014 at 11:37:25AM -0800, Jesse Barnes wrote: > On Tue, 18 Feb 2014 00:02:11 +0200 > Imre Deak wrote: > > > We can read out the pipe HW state only if the required power domain is > > on. If not we consider the pipe to be off. > > > > Signed-off-by: Imre Deak > > --- > > drivers/gpu/drm/i915/intel_display.c | 19 +++++++++++++++---- > > 1 file changed, 15 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > index ce1c00a..e3824f8 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -9535,6 +9535,18 @@ check_encoder_state(struct drm_device *dev) > > } > > } > > > > +static bool intel_display_get_pipe_config(struct intel_crtc *crtc, > > + struct intel_crtc_config *pipe_config) > > +{ > > + struct drm_i915_private *dev_priv = crtc->base.dev->dev_private; > > + > > + if (!intel_display_power_enabled(dev_priv, > > + POWER_DOMAIN_PIPE(crtc->pipe))) > > + return false; > > + > > + return dev_priv->display.get_pipe_config(crtc, pipe_config); > > +} > > + > > static void > > check_crtc_state(struct drm_device *dev) > > { > > @@ -9572,8 +9584,7 @@ check_crtc_state(struct drm_device *dev) > > "crtc's computed enabled state doesn't match tracked enabled state " > > "(expected %i, found %i)\n", enabled, crtc->base.enabled); > > > > - active = dev_priv->display.get_pipe_config(crtc, > > - &pipe_config); > > + active = intel_display_get_pipe_config(crtc, &pipe_config); > > > > /* hw state is inconsistent with the pipe A quirk */ > > if (crtc->pipe == PIPE_A && dev_priv->quirks & QUIRK_PIPEA_FORCE) > > @@ -11328,8 +11339,8 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev) > > base.head) { > > memset(&crtc->config, 0, sizeof(crtc->config)); > > > > - crtc->active = dev_priv->display.get_pipe_config(crtc, > > - &crtc->config); > > + crtc->active = intel_display_get_pipe_config(crtc, > > + &crtc->config); > > > > crtc->base.enabled = crtc->active; > > crtc->primary_enabled = crtc->active; > > Same comment about renaming .get_pipe_config applies here too, but I > think you got them all in this case, so that could be done on top. > > Reviewed-by: Jesse Barnes Same comment here - imo this should be pushed down into callbacks, since they are the only ones precisely aware of which power domains are needed. And by pushing it down we can group the power domain checks closely together with the register readback code, e.g. for fine-grained stuff like pfit or special plls. And we already have some of these checks in the hsw get_pipe_config function. So nack on this approach. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch