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@xxxxxxxxxxxxxxx>
> 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@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
