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

[Maarten Lankhorst <maarten.lankhorst@linux.intel.com>]

Op 11-06-15 om 03:35 schreef Matt Roper:
> 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?
This is temporarily, I need full conversion to atomic to kill off crtc->atomic.

After that I have 2 update functions, pre_plane_update and post_plane_update.

Any required changes could then be calculated from plane_state->visible, old_plane_state->visible and crtc enabled/disabled.
> <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.
>
Yeah, I only wanted to preserve current behavior in this commit, so it becomes more clear why it happens.

~Maarten
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx