Re: [PATCH 2/2] drm/i915: rewrite FBC's atomic CRTC-choosing code

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

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@linux.intel.com>
> Reported-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx