Op 19-10-18 om 19:14 schreef Ville Syrjälä:
> 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@xxxxxxxxxxxxxxx>
>>>> ---
>>>> 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.
>
Ok, v5 coming up, bringing much more clarity. :)
It turns out to be easier and cleaner to remove the plane link as
a separate pass in icl_check_nv12_planes() before restoring new links.
~Maarten
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
