On Tue, May 12, 2015 at 04:43:09PM +0300, Ville Syrjälä wrote:
> On Tue, May 12, 2015 at 03:33:12PM +0200, Maarten Lankhorst wrote:
> > Op 12-05-15 om 10:18 schreef Daniel Vetter:
> > > On Mon, May 11, 2015 at 04:24:46PM +0200, Maarten Lankhorst wrote:
> > >> @@ -155,24 +163,9 @@ static int intel_plane_atomic_check(struct drm_plane *plane,
> > >> /* Clip all planes to CRTC size, or 0x0 if CRTC is disabled */
> > >> intel_state->clip.x1 = 0;
> > >> intel_state->clip.y1 = 0;
> > >> - intel_state->clip.x2 =
> > >> - crtc_state->base.active ? crtc_state->pipe_src_w : 0;
> > >> - intel_state->clip.y2 =
> > >> - crtc_state->base.active ? crtc_state->pipe_src_h : 0;
> > >> -
> > >> - /*
> > >> - * 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 (state->fb == NULL && plane->state->fb != NULL) {
> > >> - /*
> > >> - * 'prepare' is never called when plane is being disabled, so
> > >> - * we need to handle frontbuffer tracking as a special case
> > >> - */
> > >> - intel_crtc->atomic.disabled_planes |=
> > >> - (1 << drm_plane_index(plane));
> > >> - }
> > >> + drm_crtc_get_hv_timing(&crtc_state->mode,
> > >> + &intel_state->clip.x2,
> > >> + &intel_state->clip.y2);
> > > Imo this is obfuscating things a bit, why not just unconditionally copy
> > > pipe_src_w/h to clip.x/y2? get_hv_timing is for the pipe size, which on
> > > most platforms must match the primary plane window except for gen2 and
> > > gen9+.
> > pipe_src_* is calculated in the same way,
>
> Except we may end up rounding pipe_src_w down if we have
> double-wide/dual link lvds etc., and we definitely need to clip the
> planes against the real pipe src size.
>
> > and at the time of plane validation the crtc validation hasn't run yet,
> > so pipe_src_* contains outdated values which break in interesting ways.
>
> Just check crtc first. Or actually we probably need pre + post crtc
> checks since we want to do wm compute and such after the planes have
> been handled.
Yeah in the end the sequence should be:
1. compute pipe_config for modeset changes on any crtcs where mode_changed
is set.
2. call helper_check_planes which should use the values computed in step
1. to compute the nuclear plane flip states. It's important that the
depencies flow one-way since only then can we skip all the modeset
state recomputation (and possible serialization because we need more
crtc states) for pure plane flips.
Where exactly do the pipe_src_w/h values get out of sync here? Imo better
to patch them up someplace in the legacy modeset code than add hacks to
plane atomic functions.
Cheers, Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
