Re: [PATCH v2 12/27] drm/i915: Split plane updates of crtc->atomic into a helper, v2.

[Matt Roper <matthew.d.roper@intel.com>]

On Thu, Jun 04, 2015 at 02:47:42PM +0200, Maarten Lankhorst wrote:
> This makes it easier to verify that no changes are done when
> calling this from crtc instead.
> 
> Changes since v1:
>  - Make intel_wm_need_update static and always check it.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

Do we even need crtc->atomic anymore?  When I first added that, I
expected it to just be a temporary dumping ground until we had
crtc_state's tracked and swapped properly (which we do now).  Can we
just migrate these fields into the state structure instead?


<snip>
> +int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
> +				    struct drm_plane_state *plane_state)
> +{
> +	struct drm_crtc *crtc = crtc_state->crtc;
> +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +	struct drm_plane *plane = plane_state->plane;
> +	struct drm_device *dev = crtc->dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct intel_plane_state *old_plane_state =
> +		to_intel_plane_state(plane->state);
> +	int idx = intel_crtc->base.base.id, ret;
> +	int i = drm_plane_index(plane);
> +	bool mode_changed = needs_modeset(crtc_state);
> +	bool was_crtc_enabled = crtc->state->active;
> +	bool is_crtc_enabled = crtc_state->active;
> +
> +	bool turn_off, turn_on, visible, was_visible;
> +	struct drm_framebuffer *fb = plane_state->fb;
> +
> +	if (crtc_state && INTEL_INFO(dev)->gen >= 9 &&
> +	    plane->type != DRM_PLANE_TYPE_CURSOR) {
> +		ret = skl_update_scaler_plane(
> +			to_intel_crtc_state(crtc_state),
> +			to_intel_plane_state(plane_state));
> +		if (ret)
> +			return ret;
> +	}
> +
> +	/*
> +	 * Disabling a plane is always okay; we just need to update
> +	 * fb tracking in a special way since cleanup_fb() won't
> +	 * get called by the plane helpers.
> +	 */
> +	if (old_plane_state->base.fb && !fb)
> +		intel_crtc->atomic.disabled_planes |= 1 << i;
> +
> +	/* don't run rest during modeset yet */
> +	if (!intel_crtc->active || mode_changed)
> +		return 0;
> +
> +	was_visible = old_plane_state->visible;
> +	visible = to_intel_plane_state(plane_state)->visible;
> +
> +	if (!was_crtc_enabled && WARN_ON(was_visible))
> +		was_visible = false;
> +
> +	if (!is_crtc_enabled && WARN_ON(visible))
> +		visible = false;
> +
> +	if (!was_visible && !visible)
> +		return 0;
> +
> +	turn_off = was_visible && (!visible || mode_changed);
> +	turn_on = visible && (!was_visible || mode_changed);
> +
> +	DRM_DEBUG_ATOMIC("[CRTC:%i] has [PLANE:%i] with fb %i\n", idx,
> +			 plane->base.id, fb ? fb->base.id : -1);
> +
> +	DRM_DEBUG_ATOMIC("[PLANE:%i] visible %i -> %i, off %i, on %i, ms %i\n",
> +			 plane->base.id, was_visible, visible,
> +			 turn_off, turn_on, mode_changed);
> +
> +	if (intel_wm_need_update(plane, plane_state))
> +		intel_crtc->atomic.update_wm = true;

Hmm, I realize this is the way the code already worked, but this is only
going to trigger update_watermarks rather than update_sprite_watermarks,
which on some platforms could make us update watermarks with stale
sprite values.  I think the only reason we get away with this today is
because we actually perform sprite watermark updates in the low-level
plane update functions (which is bad since we're under vblank
evasion...).  I had some patches that fixed that oversight as part of my
watermark RFC series, but they haven't gone in; I probably need to
extract the fix from the rest of the RFC.

Not sure if you want to worry about it as part of your work here or not
since this doesn't leave us any worse off than we already are today;
just figured I'd mention it so we don't forget about it.


Matt

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx