[Intel-gfx] [PATCH v2 3/8] drm/i915/gen11: Link nv12 Y and UV planes in the atomic state, v4.
Ville Syrjälä
ville.syrjala at linux.intel.com
Fri Oct 19 17:14:03 UTC 2018
On Fri, Oct 19, 2018 at 04:22:29PM +0200, Maarten Lankhorst wrote:
> Op 18-10-18 om 18:00 schreef Ville Syrjälä:
> > On Thu, Oct 18, 2018 at 01:51:29PM +0200, Maarten Lankhorst wrote:
> >> To make NV12 working on icl, we need to update 2 planes simultaneously.
> >> I've chosen to do this in the CRTC step after plane validation is done,
> >> so we know what planes are (in)visible. The linked Y plane will get
> >> updated in intel_plane_update_planes_on_crtc(), by the call to
> >> update_slave, which gets the master's plane_state as argument.
> >>
> >> The link requires both planes for atomic_update to work,
> >> so make sure skl_ddb_add_affected_planes() adds both states.
> >>
> >> Changes since v1:
> >> - Introduce icl_is_nv12_y_plane(), instead of hardcoding sprite numbers.
> >> - Put all the state updating login in intel_plane_atomic_check_with_state().
> >> - Clean up changes in intel_plane_atomic_check().
> >> Changes since v2:
> >> - Fix intel_atomic_get_old_plane_state() to actually return old state.
> >> - Move visibility changes to preparation patch.
> >> - Only try to find a Y plane on gen11, earlier platforms only require
> >> a single plane.
> >> Changes since v3:
> >> - Fix checkpatch warning about to_intel_crtc() usage.
> >> - Add affected planes from icl_add_linked_planes() before check_planes(),
> >> it's a cleaner way to do this. (Ville)
> >>
> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
> >> ---
> >> drivers/gpu/drm/i915/intel_atomic_plane.c | 74 ++++++++++++++++-----
> >> drivers/gpu/drm/i915/intel_display.c | 81 +++++++++++++++++++++++
> >> drivers/gpu/drm/i915/intel_drv.h | 53 +++++++++++++++
> >> drivers/gpu/drm/i915/intel_pm.c | 12 +++-
> >> 4 files changed, 202 insertions(+), 18 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
> >> index b957ad63cd87..154ea3dc344f 100644
> >> --- a/drivers/gpu/drm/i915/intel_atomic_plane.c
> >> +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
> >> @@ -122,7 +122,11 @@ int intel_plane_atomic_check_with_state(const struct intel_crtc_state *old_crtc_
> >> crtc_state->nv12_planes &= ~BIT(intel_plane->id);
> >> intel_state->base.visible = false;
> >>
> >> - /* If this is a cursor plane, no further checks are needed. */
> >> + /* Destroy the link */
> >> + intel_state->linked_plane = NULL;
> >> + intel_state->slave = false;
> >> +
> >> + /* If this is a cursor or Y plane, no further checks are needed. */
> >> if (!intel_state->base.crtc && !old_plane_state->base.crtc)
> >> return 0;
> >>
> >> @@ -143,27 +147,44 @@ int intel_plane_atomic_check_with_state(const struct intel_crtc_state *old_crtc_
> >> state);
> >> }
> >>
> >> -static int intel_plane_atomic_check(struct drm_plane *plane,
> >> - struct drm_plane_state *new_plane_state)
> >> +static int intel_plane_atomic_check(struct drm_plane *drm_plane,
> >> + struct drm_plane_state *new_drm_plane_state)
> > My new cunning plane is to call these _plane, _new_plane_state etc.
> > It should discourage people from using them and the aliasing
> > intel_ types at the same time. And it avoids polluting the namespace
> > for things we don't really want to use.
> >
> > I already snuck in some uses of this ;)
> >
> >> {
> >> - struct drm_atomic_state *state = new_plane_state->state;
> >> - const struct drm_plane_state *old_plane_state =
> >> - drm_atomic_get_old_plane_state(state, plane);
> >> - struct drm_crtc *crtc = new_plane_state->crtc ?: old_plane_state->crtc;
> >> - const struct drm_crtc_state *old_crtc_state;
> >> - struct drm_crtc_state *new_crtc_state;
> >> -
> >> - new_plane_state->visible = false;
> >> + struct intel_atomic_state *state =
> >> + to_intel_atomic_state(new_drm_plane_state->state);
> >> + struct intel_plane *plane = to_intel_plane(drm_plane);
> >> + const struct intel_plane_state *old_plane_state =
> >> + intel_atomic_get_old_plane_state(state, plane);
> >> + struct intel_plane_state *new_plane_state =
> >> + to_intel_plane_state(new_drm_plane_state);
> >> + struct intel_crtc *crtc =
> >> + to_intel_crtc(new_plane_state->base.crtc ?:
> >> + old_plane_state->base.crtc);
> >> + const struct intel_crtc_state *old_crtc_state;
> >> + struct intel_crtc_state *new_crtc_state;
> >> + struct intel_plane *linked = old_plane_state->linked_plane;
> >> +
> >> + if (linked && !crtc) {
> >> + const struct intel_plane_state *old_linked_state =
> >> + intel_atomic_get_old_plane_state(state, linked);
> > Plane without a crtc that happens to have a linked plane
> > attached to it...
> >
> > I guess that implies that 'plane' here is the slave and
> > it was already active during the previous state (otherwise
> > it would not have been linked to the other plane). So that
> > means the master plane must have a valid crtc in its old
> > plane state.
> >
> > Did I decode that correctly?
> Correct.
> > Maybe what we want to do here is to just always clear the
> > active_planes bit for the slave in the master plane's
> > old crtc's new crtc state (quite the mouthful), and then
> > run through the normal check_plane stuff for the slave
> > with its own crtc (if it has one). In practice it doesn't
> > really make any difference I suppose since our planes
> > can't move between crtcs, but logically it would make
> > more sense to me.
> Even if we could move planes, we can't move planes between CRTC's in a single atomic commit.
> First comes the disabling, then comes the moving.
>
> I think it's less of a mess of doing it this way, it keeps intel_plane_atomic_check_with_state() obvious.
It's rather non-obvious. I had to think about it for a while.
I think it would be much clearer to keep it all in one self
contained function instead of spreading it across several
functions.
remove_plane_link()
{
if (plane_state->linked && plane_state->slave) {
old_linked_state = get_old_plane_state(plane_state->linked);
new_crtc_state = get_new_crtc_state(old_linked_state->crtc);
new_crtc_state->active_planes &= ~BIT(plane->id);
}
plane_state->linked = NULL;
plane_state->slave = false;
}
or something.
--
Ville Syrjälä
Intel
More information about the Intel-gfx
mailing list