From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Subject: Re: [PATCH 78/89] drm/i915/skl: Flush the WM configuration Date: Fri, 19 Sep 2014 13:46:01 +0300 Message-ID: <20140919104601.GH12416@intel.com> References: <1409830075-11139-1-git-send-email-damien.lespiau@intel.com> <1409830075-11139-79-git-send-email-damien.lespiau@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by gabe.freedesktop.org (Postfix) with ESMTP id 8B0886E14B for ; Fri, 19 Sep 2014 03:46:26 -0700 (PDT) Content-Disposition: inline In-Reply-To: <1409830075-11139-79-git-send-email-damien.lespiau@intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Damien Lespiau Cc: intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org On Thu, Sep 04, 2014 at 12:27:44PM +0100, Damien Lespiau wrote: > When we write new values for the DDB allocation and WM parameters, we now > need to trigger the double buffer update for the pipe to take the new > configuration into account. > = > As the DDB is a global resource shared between planes, enabling or > disabling one plane will result in changes for all planes that are > currently in use, thus the need write PLANE_SURF/CUR_BASE for more than > the plane we're touching. Ah, so here's the sequenced DDB update logic I was looking for. > = > v2: Don't wait for pipes that are off > = > v3: Split the staging results structure to not exceed the 1Kb stack > allocation in skl_update_wm() > = > Signed-off-by: Damien Lespiau > --- > drivers/gpu/drm/i915/intel_pm.c | 92 +++++++++++++++++++++++++++++++++++= ++++++ > 1 file changed, 92 insertions(+) > = > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel= _pm.c > index 7f7a2e2..d378879 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -3198,6 +3198,22 @@ static bool skl_ddb_allocation_changed(const struc= t skl_ddb_allocation *new_ddb, > return false; > } > = > +static unsigned int > +skl_ddb_pipe_allocation_size(const struct skl_ddb_allocation *ddb, > + const struct intel_crtc *intel_crtc) > +{ > + struct drm_device *dev =3D intel_crtc->base.dev; > + unsigned int size =3D 0; > + enum pipe pipe =3D intel_crtc->pipe; > + int plane; > + > + for_each_plane(pipe, plane) > + size +=3D skl_ddb_entry_size(&ddb->plane[pipe][plane]); > + size +=3D skl_ddb_entry_size(&ddb->cursor[pipe]); > + > + return size; > +} > + > static void skl_compute_wm_global_parameters(struct drm_device *dev, > struct intel_wm_config *config) > { > @@ -3434,6 +3450,81 @@ static void skl_write_wm_values(struct drm_i915_pr= ivate *dev_priv, > } > } > = > +static void skl_wm_flush_pipe(struct drm_i915_private *dev_priv, enum pi= pe pipe) > +{ > + struct drm_device *dev =3D dev_priv->dev; > + int plane; > + > + for_each_plane(pipe, plane) { > + I915_WRITE(PLANE_SURF(pipe, plane), > + I915_READ(PLANE_SURF(pipe, plane))); > + } > + I915_WRITE(CURBASE(pipe), I915_READ(CURBASE(pipe))); > +} I'm not sure I really like this thing. The DDB/wm update should be part of the atomic pipe update. But since we're not really there yet I guess we need to start with something. And dealing with multiple pipes is a definite complication here. > + > +static void skl_flush_wm_values(struct drm_i915_private *dev_priv, > + struct skl_wm_values *new_values) > +{ > + struct drm_device *dev =3D dev_priv->dev; > + struct skl_ddb_allocation *cur_ddb, *new_ddb; > + unsigned int cur_size[I915_MAX_PIPES], new_size[I915_MAX_PIPES]; > + struct intel_crtc *crtc; > + enum pipe pipe; > + > + new_ddb =3D &new_values->ddb; > + cur_ddb =3D &dev_priv->wm.skl_hw.ddb; > + > + /* > + * Start by computing the total allocated space for each pipe as we > + * need that values for the two passes. > + */ > + for_each_intel_crtc(dev, crtc) { > + pipe =3D crtc->pipe; > + new_size[pipe] =3D skl_ddb_pipe_allocation_size(new_ddb, crtc); > + cur_size[pipe] =3D skl_ddb_pipe_allocation_size(cur_ddb, crtc); > + } > + > + /* > + * First pass: we flush the pipes that had their allocation reduced. > + * > + * We then have to wait until the pipe stops fetching pixels from the > + * previous allocation. This way, pipes that have just been allocated > + * more space won't try to fetch pixels belonging to a different pipe. > + */ > + for_each_intel_crtc(dev, crtc) { > + if (!crtc->active) > + continue; > + > + pipe =3D crtc->pipe; > + > + if (new_size[pipe] < cur_size[pipe]) { > + skl_wm_flush_pipe(dev_priv, pipe); > + intel_wait_for_vblank(dev, pipe); > + } > + } > + > + /* > + * Second pass: flush the pipes that got more allocated space. > + * > + * We don't need to actively wait for the update here, next vblank > + * will just get more DDB space with the correct WM values. > + */ > + for_each_intel_crtc(dev, crtc) { > + if (!crtc->active) > + continue; > + > + pipe =3D crtc->pipe; > + > + if (new_size[pipe] < cur_size[pipe]) > + continue; > + > + if (!skl_ddb_allocation_changed(new_ddb, crtc)) > + continue; > + > + skl_wm_flush_pipe(dev_priv, pipe); > + } > +} I don't think this logic will do the right thing. Consider for example when going from two pipes to three: 1. initially DDB looks like this | B | C | 2. enable pipe A 3. reduce pipe B DDB allocation | | B..| | | | C | Notice the part marked with .. is now used by both pipes B and C until the allocation for C also gets reduced. It would work correctly in case we would go AB->ABC or AC->ABC. Similar problem would be encountered when going ABC->AB. So we need more care in which order we update the DDB allocation for each pipe to avoid such overlaps. > + > static bool skl_update_pipe_wm(struct drm_crtc *crtc, > struct skl_pipe_wm_parameters *params, > struct intel_wm_config *config, > @@ -3525,6 +3616,7 @@ static void skl_update_wm(struct drm_crtc *crtc) > = > skl_update_other_pipe_wm(dev, crtc, &config, results); > skl_write_wm_values(dev_priv, results); > + skl_flush_wm_values(dev_priv, results); > = > /* store the new configuration */ > dev_priv->wm.skl_hw =3D *results; > -- = > 1.8.3.1 > = > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- = Ville Syrj=E4l=E4 Intel OTC