On Mon, 09 Jun 2014, Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> wrote:
> On Wed, Jun 04, 2014 at 06:22:13PM +0200, Daniel Vetter wrote:
>> On Tue, Jun 03, 2014 at 05:51:01PM -0300, Paulo Zanoni wrote:
>> > 2014-05-22 11:48 GMT-03:00 <ville.syrjala@xxxxxxxxxxxxxxx>:
>> > > From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
>> > >
>> > > We need to perform watermark programming before and after changing the
>> > > plane configuration. Add two new vfuncs to do that. The pre phase is
>> > > supposed to switch over to the intermediate watermarks which are
>> > > computed so that they can deal with both the old and new plane
>> > > configurations. The post phase will arm the vblank based update
>> > > systems to switch over to the optimal target watermarks after the
>> > > plane configuration has for sure changed.
>> > >
>> > > v2: Pass around intel_crtc and s/intel_crtc/crtc/
>> > >
>> > > Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
>> > > ---
>> > > drivers/gpu/drm/i915/i915_drv.h | 5 +++
>> > > drivers/gpu/drm/i915/intel_drv.h | 11 +++++
>> > > drivers/gpu/drm/i915/intel_pm.c | 88 ++++++++++++++++++++++++++++++++++++++++
>> > > 3 files changed, 104 insertions(+)
>> > >
>> > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> > > index c90d5ac..d4f8ae8 100644
>> > > --- a/drivers/gpu/drm/i915/i915_drv.h
>> > > +++ b/drivers/gpu/drm/i915/i915_drv.h
>> > > @@ -407,6 +407,7 @@ struct intel_plane_config;
>> > > struct intel_crtc;
>> > > struct intel_limit;
>> > > struct dpll;
>> > > +struct intel_crtc_wm_config;
>> > >
>> > > struct drm_i915_display_funcs {
>> > > bool (*fbc_enabled)(struct drm_device *dev);
>> > > @@ -437,6 +438,10 @@ struct drm_i915_display_funcs {
>> > > struct drm_crtc *crtc,
>> > > uint32_t sprite_width, int pixel_size,
>> > > bool enable, bool scaled);
>> > > + void (*program_wm_pre)(struct intel_crtc *crtc,
>> > > + const struct intel_crtc_wm_config *config);
>> > > + void (*program_wm_post)(struct intel_crtc *crtc,
>> > > + const struct intel_crtc_wm_config *config);
>> > > void (*modeset_global_resources)(struct drm_device *dev);
>> > > /* Returns the active state of the crtc, and if the crtc is active,
>> > > * fills out the pipe-config with the hw state. */
>> > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> > > index 72f01b1..4b59be3 100644
>> > > --- a/drivers/gpu/drm/i915/intel_drv.h
>> > > +++ b/drivers/gpu/drm/i915/intel_drv.h
>> > > @@ -358,6 +358,13 @@ struct intel_pipe_wm {
>> > > bool sprites_scaled;
>> > > };
>> > >
>> > > +struct intel_crtc_wm_config {
>> > > + /* target watermarks for the pipe */
>> > > + struct intel_pipe_wm target;
>> > > + /* intermediate watermarks for pending/active->target transition */
>> > > + struct intel_pipe_wm intm;
>> >
>> > It seems you always prefer shorter names such as "intm", and I usually
>> > prefer the longer ones like "intermediate". Looks like this is a
>> > common topic for my bikesheddings on your patches. When I read "intm"
>> > my brain parses it as "Int M" and then aborts execution =P
>>
>> I agree with Paulo here. Some other name suggestion (since intermediate is
>> so long): transition/transit, merged, pending, ...
>
> The two good names I could think of "intermediate" and "transitional"
> are both really long. I agree that "intm" is a horrible shorthand,
> but I couldn't come up with anything half sane and still reasonably
> short.
>
> "merged" and "pending" already appear in the code with different
> meaning, so I'd rather not confuse the matter by reusing those.
> "transition" seems OK to me, though not that short either.
> "transit" brings up wrong kinds of images in my brain so i don't
> really like it.
interim, transient, temporary, or simly just temp?
BR,
Jani.
>
>> -Daniel
>>
>> >
>> > With or without that changed: Reviewed-by: Paulo Zanoni
>> > <paulo.r.zanoni@xxxxxxxxx>
>> >
>> > > +};
>> > > +
>> > > struct intel_crtc {
>> > > struct drm_crtc base;
>> > > enum pipe pipe;
>> > > @@ -1001,6 +1008,10 @@ void intel_init_runtime_pm(struct drm_i915_private *dev_priv);
>> > > void intel_fini_runtime_pm(struct drm_i915_private *dev_priv);
>> > > void ilk_wm_get_hw_state(struct drm_device *dev);
>> > > void ilk_update_pipe_wm(struct drm_device *dev, enum pipe pipe);
>> > > +void intel_program_watermarks_pre(struct intel_crtc *crtc,
>> > > + const struct intel_crtc_wm_config *config);
>> > > +void intel_program_watermarks_post(struct intel_crtc *crtc,
>> > > + const struct intel_crtc_wm_config *config);
>> > >
>> > >
>> > > /* intel_sdvo.c */
>> > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
>> > > index 6fc6416..ccf920a 100644
>> > > --- a/drivers/gpu/drm/i915/intel_pm.c
>> > > +++ b/drivers/gpu/drm/i915/intel_pm.c
>> > > @@ -2950,6 +2950,20 @@ void ilk_update_pipe_wm(struct drm_device *dev, enum pipe pipe)
>> > > spin_unlock(&crtc->wm.lock);
>> > > }
>> > >
>> > > +static void ilk_wm_cancel(struct intel_crtc *crtc)
>> > > +{
>> > > + struct drm_device *dev = crtc->base.dev;
>> > > +
>> > > + assert_spin_locked(&crtc->wm.lock);
>> > > +
>> > > + crtc->wm.dirty = false;
>> > > +
>> > > + if (crtc->wm.vblank) {
>> > > + drm_vblank_put(dev, crtc->pipe);
>> > > + crtc->wm.vblank = false;
>> > > + }
>> > > +}
>> > > +
>> > > static void ilk_update_sprite_wm(struct drm_plane *plane,
>> > > struct drm_crtc *crtc,
>> > > uint32_t sprite_width, int pixel_size,
>> > > @@ -3084,6 +3098,24 @@ void intel_update_sprite_watermarks(struct drm_plane *plane,
>> > > pixel_size, enabled, scaled);
>> > > }
>> > >
>> > > +void intel_program_watermarks_pre(struct intel_crtc *crtc,
>> > > + const struct intel_crtc_wm_config *config)
>> > > +{
>> > > + struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
>> > > +
>> > > + if (dev_priv->display.program_wm_pre)
>> > > + dev_priv->display.program_wm_pre(crtc, config);
>> > > +}
>> > > +
>> > > +void intel_program_watermarks_post(struct intel_crtc *crtc,
>> > > + const struct intel_crtc_wm_config *config)
>> > > +{
>> > > + struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
>> > > +
>> > > + if (dev_priv->display.program_wm_post)
>> > > + dev_priv->display.program_wm_post(crtc, config);
>> > > +}
>> > > +
>> > > static struct drm_i915_gem_object *
>> > > intel_alloc_context_page(struct drm_device *dev)
>> > > {
>> > > @@ -6522,6 +6554,60 @@ void intel_fini_runtime_pm(struct drm_i915_private *dev_priv)
>> > > pm_runtime_disable(device);
>> > > }
>> > >
>> > > +static void ilk_program_wm_pre(struct intel_crtc *crtc,
>> > > + const struct intel_crtc_wm_config *config)
>> > > +{
>> > > + struct drm_device *dev = crtc->base.dev;
>> > > + struct drm_i915_private *dev_priv = dev->dev_private;
>> > > +
>> > > + mutex_lock(&dev_priv->wm.mutex);
>> > > +
>> > > + spin_lock_irq(&crtc->wm.lock);
>> > > + ilk_wm_cancel(crtc);
>> > > + spin_unlock_irq(&crtc->wm.lock);
>> > > +
>> > > + /* pending update (if any) got cancelled */
>> > > + crtc->wm.pending = crtc->wm.active;
>> > > +
>> > > + if (!memcmp(&crtc->wm.active, &config->intm, sizeof(config->intm)))
>> > > + goto unlock;
>> > > +
>> > > + crtc->wm.active = config->intm;
>> > > +
>> > > + /* use the most up to date watermarks for other pipes */
>> > > + ilk_refresh_pending_watermarks(dev);
>> > > +
>> > > + /* switch over to the intermediate watermarks */
>> > > + ilk_program_watermarks(dev);
>> > > +
>> > > + unlock:
>> > > + mutex_unlock(&dev_priv->wm.mutex);
>> > > +}
>> > > +
>> > > +static void ilk_program_wm_post(struct intel_crtc *crtc,
>> > > + const struct intel_crtc_wm_config *config)
>> > > +{
>> > > + struct drm_device *dev = crtc->base.dev;
>> > > + struct drm_i915_private *dev_priv = dev->dev_private;
>> > > + u32 vbl_count;
>> > > +
>> > > + /*
>> > > + * FIXME sample this inside the atomic section to avoid
>> > > + * needlessly long periods w/ sub-par watermarks
>> > > + */
>> > > + vbl_count = dev->driver->get_vblank_counter(dev, crtc->pipe);
>> > > +
>> > > + mutex_lock(&dev_priv->wm.mutex);
>> > > +
>> > > + /*
>> > > + * We can switch over to the target
>> > > + * watermarks after the next vblank.
>> > > + */
>> > > + ilk_setup_pending_watermarks(crtc, &config->target, vbl_count);
>> > > +
>> > > + mutex_unlock(&dev_priv->wm.mutex);
>> > > +}
>> > > +
>> > > /* Set up chip specific power management-related functions */
>> > > void intel_init_pm(struct drm_device *dev)
>> > > {
>> > > @@ -6569,6 +6655,8 @@ void intel_init_pm(struct drm_device *dev)
>> > > dev_priv->wm.spr_latency[0] && dev_priv->wm.cur_latency[0])) {
>> > > dev_priv->display.update_wm = ilk_update_wm;
>> > > dev_priv->display.update_sprite_wm = ilk_update_sprite_wm;
>> > > + dev_priv->display.program_wm_pre = ilk_program_wm_pre;
>> > > + dev_priv->display.program_wm_post = ilk_program_wm_post;
>> > > } else {
>> > > DRM_DEBUG_KMS("Failed to read display plane latency. "
>> > > "Disable CxSR\n");
>> > > --
>> > > 1.8.5.5
>> > >
>> > > _______________________________________________
>> > > Intel-gfx mailing list
>> > > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
>> > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>> >
>> >
>> >
>> > --
>> > Paulo Zanoni
>> > _______________________________________________
>> > Intel-gfx mailing list
>> > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
>> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>>
>> --
>> Daniel Vetter
>> Software Engineer, Intel Corporation
>> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
>
> --
> Ville Syrjälä
> Intel OTC
> _______________________________________________
> 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
