Hey, Op 26-12-16 om 19:45 schreef Paulo Zanoni: > Ville pointed out that intel_fbc_choose_crtc() is iterating over all > planes instead of just the primary planes. There are no real > consequences of this problem for HSW+, and for the other platforms it > just means that in some obscure multi-screen cases we'll keep FBC > disabled when we could have enabled it. Still, iterating over all > planes doesn't seem to be the best thing to do. > > My initial idea was to just add a check for plane->type and be done, > but then I realized that in commits not involving the primary plane we > would reset crtc_state->enable_fbc back to false even when FBC is > enabled. That also wouldn't result in a bug due to the way the > enable_fbc variable is checked, but, still, our code can be better > than this. > > So I went for the solution that involves tracking enable_fbc in the > primary plane state instead of the CRTC state. This way, if a commit > doesn't involve the primary plane for the CRTC we won't be resetting > enable_fbc back to false, so the variable will always reflect the > reality. And this change also makes more sense since FBC is actually > tied to the single plane and not the full pipe. As a bonus, we only > iterate over the CRTCs instead of iterating over all planes. > > v2: > > But Ville pointed that this only works properly if we have a single > commit with multiple CRTCs. If we have multiple parallel commits, each > with its own CRTC, we'll end with enable_fbc set to true in more than > one plane. > > So the solution was to rename enable_fbc to can_enable_fbc. If we just > did the rename as the patch was, we'd end up with a single plane with > can_enable_fbc on commits involving multiple CRTCs: we'd choose the > best one, but we'd still end up with a variable that doesn't 100% > reflect reality. > > So in the end I adopted Ville's suggestion of the fbc_score variable. > And then, instead of checking the score at intel_fbc_choose_crtc() > it should be possible to check for the score at intel_fbc_enable(). > This allows us to simplify intel_fbc_choose_crtc() to the point where > it only needs to look at the given plane in order to assign its score > instead of looking at every primary plane on the same commit. > > We still only set scores of 0 and 1 and we don't really do the > score-checking loop. > > v3: Rebase. > > v4: Move intel_fbc_check_plane() call up so it will still zero the > score if the plane has no FB. (Ville). > > v5: Fix the segfault introduced by the previous version and add a > WARN to validate our current assumptions. > > Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> > Reported-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx> I'm hitting the WARN_ON(plane_state->fbc_score == 0) on my gen9 when running kms_atomic_transition.plane-* tests. This needs some more fixing to work with cases where the primary plane is disabled. :-) ~Maarten _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
