[Intel-gfx] [PATCH v11 10/12] drm/i915: Link planes in a bigjoiner configuration, v3.
Maarten Lankhorst
maarten.lankhorst at linux.intel.com
Wed Oct 28 12:26:27 UTC 2020
Op 27-10-2020 om 20:11 schreef Ville Syrjälä:
> On Tue, Oct 27, 2020 at 11:19:16AM -0700, Navare, Manasi wrote:
>> On Tue, Oct 27, 2020 at 03:42:30PM +0200, Ville Syrjälä wrote:
>>> On Mon, Oct 26, 2020 at 03:41:48PM -0700, Navare, Manasi wrote:
>>>> On Mon, Oct 26, 2020 at 10:18:54PM +0200, Ville Syrjälä wrote:
>>>>> On Wed, Oct 21, 2020 at 10:42:21PM -0700, Manasi Navare wrote:
>>>>>> From: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
>>>>>>
>>>>>> Make sure that when a plane is set in a bigjoiner mode, we will add
>>>>>> their counterpart to the atomic state as well. This will allow us to
>>>>>> make sure all state is available when planes are checked.
>>>>>>
>>>>>> Because of the funny interactions with bigjoiner and planar YUV
>>>>>> formats, we may end up adding a lot of planes, so we have to keep
>>>>>> iterating until we no longer add any planes.
>>>>>>
>>>>>> Also fix the atomic intel plane iterator, so things watermarks start
>>>>>> working automagically.
>>>>>>
>>>>>> v6:
>>>>>> * Fix from_plane_state assignments (Manasi)
>>>>>> v5:
>>>>>> * Rebase after adding sagv support (Manasi)
>>>>>> v4:
>>>>>> * Manual rebase (Manasi)
>>>>>> Changes since v1:
>>>>>> - Rebase on top of plane_state split, cleaning up the code a lot.
>>>>>> - Make intel_atomic_crtc_state_for_each_plane_state() bigjoiner capable.
>>>>>> - Add iter macro to intel_atomic_crtc_state_for_each_plane_state() to
>>>>>> keep iteration working.
>>>>>> Changes since v2:
>>>>>> - Add icl_(un)set_bigjoiner_plane_links, to make it more clear where
>>>>>> links are made and broken.
>>>>>>
>>>>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
>>>>>> Signed-off-by: Manasi Navare <manasi.d.navare at intel.com>
>>>>>> ---
>>>>>> .../gpu/drm/i915/display/intel_atomic_plane.c | 53 ++++-
>>>>>> .../gpu/drm/i915/display/intel_atomic_plane.h | 3 +-
>>>>>> drivers/gpu/drm/i915/display/intel_display.c | 207 ++++++++++++++++--
>>>>>> drivers/gpu/drm/i915/display/intel_display.h | 20 +-
>>>>>> .../drm/i915/display/intel_display_types.h | 11 +
>>>>>> drivers/gpu/drm/i915/intel_pm.c | 20 +-
>>>>>> 6 files changed, 274 insertions(+), 40 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
>>>>>> index 3334ff253600..5df928f8f322 100644
>>>>>> --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c
>>>>>> +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
>>>>>> @@ -246,12 +246,17 @@ static void intel_plane_clear_hw_state(struct intel_plane_state *plane_state)
>>>>>> memset(&plane_state->hw, 0, sizeof(plane_state->hw));
>>>>>> }
>>>>>>
>>>>>> -void intel_plane_copy_uapi_to_hw_state(struct intel_plane_state *plane_state,
>>>>>> +void intel_plane_copy_uapi_to_hw_state(const struct intel_crtc_state *crtc_state,
>>>>>> + struct intel_plane_state *plane_state,
>>>>>> const struct intel_plane_state *from_plane_state)
>>>>>> {
>>>>>> intel_plane_clear_hw_state(plane_state);
>>>>>>
>>>>>> - plane_state->hw.crtc = from_plane_state->uapi.crtc;
>>>>>> + if (from_plane_state->uapi.crtc)
>>>>>> + plane_state->hw.crtc = crtc_state->uapi.crtc;
>>>>>> + else
>>>>>> + plane_state->hw.crtc = NULL;
>>>>>> +
>>>>>> plane_state->hw.fb = from_plane_state->uapi.fb;
>>>>>> if (plane_state->hw.fb)
>>>>>> drm_framebuffer_get(plane_state->hw.fb);
>>>>>> @@ -320,15 +325,36 @@ int intel_plane_atomic_check_with_state(const struct intel_crtc_state *old_crtc_
>>>>>> }
>>>>>>
>>>>>> static struct intel_crtc *
>>>>>> -get_crtc_from_states(const struct intel_plane_state *old_plane_state,
>>>>>> +get_crtc_from_states(struct intel_atomic_state *state,
>>>>>> + const struct intel_plane_state *old_plane_state,
>>>>>> const struct intel_plane_state *new_plane_state)
>>>>>> {
>>>>>> + struct drm_i915_private *dev_priv = to_i915(state->base.dev);
>>>>>> + struct intel_plane *plane = to_intel_plane(new_plane_state->uapi.plane);
>>>>>> +
>>>>>> if (new_plane_state->uapi.crtc)
>>>>>> return to_intel_crtc(new_plane_state->uapi.crtc);
>>>>>>
>>>>>> if (old_plane_state->uapi.crtc)
>>>>>> return to_intel_crtc(old_plane_state->uapi.crtc);
>>>>>>
>>>>>> + if (new_plane_state->bigjoiner_slave) {
>>>>>> + const struct intel_plane_state *new_master_plane_state =
>>>>>> + intel_atomic_get_new_plane_state(state, new_plane_state->bigjoiner_plane);
>>>>>> +
>>>>>> + /* need to use uapi here, new_master_plane_state might not be copied to hw yet */
>>>>>> + if (new_master_plane_state->uapi.crtc)
>>>>>> + return intel_get_crtc_for_pipe(dev_priv, plane->pipe);
>>>>>> + }
>>>>>> +
>>>>>> + if (old_plane_state->bigjoiner_slave) {
>>>>>> + const struct intel_plane_state *old_master_plane_state =
>>>>>> + intel_atomic_get_old_plane_state(state, old_plane_state->bigjoiner_plane);
>>>>>> +
>>>>>> + if (old_master_plane_state->uapi.crtc)
>>>>>> + return intel_get_crtc_for_pipe(dev_priv, plane->pipe);
>>>>>> + }
>>>>>> +
>>>>>> return NULL;
>>>>>> }
>>>>>>
>>>>>> @@ -339,18 +365,33 @@ int intel_plane_atomic_check(struct intel_atomic_state *state,
>>>>>> intel_atomic_get_new_plane_state(state, plane);
>>>>>> const struct intel_plane_state *old_plane_state =
>>>>>> intel_atomic_get_old_plane_state(state, plane);
>>>>>> + const struct intel_plane_state *new_master_plane_state;
>>>>>> struct intel_crtc *crtc =
>>>>>> - get_crtc_from_states(old_plane_state, new_plane_state);
>>>>>> + get_crtc_from_states(state, old_plane_state,
>>>>>> + new_plane_state);
>>>>>> const struct intel_crtc_state *old_crtc_state;
>>>>>> struct intel_crtc_state *new_crtc_state;
>>>>>>
>>>>>> - intel_plane_copy_uapi_to_hw_state(new_plane_state, new_plane_state);
>>>>>> + if (crtc)
>>>>>> + new_crtc_state = intel_atomic_get_new_crtc_state(state, crtc);
>>>>>> + else
>>>>>> + new_crtc_state = NULL;
>>>>>> +
>>>>>> + new_master_plane_state = new_plane_state;
>>>>>> + if (new_plane_state->bigjoiner_slave)
>>>>>> + new_master_plane_state =
>>>>>> + intel_atomic_get_new_plane_state(state,
>>>>>> + new_plane_state->bigjoiner_plane);
>>>>>> +
>>>>>> + intel_plane_copy_uapi_to_hw_state(new_crtc_state,
>>>>>> + new_plane_state,
>>>>>> + new_master_plane_state);
>>>>>> +
>>>>>> new_plane_state->uapi.visible = false;
>>>>>> if (!crtc)
>>>>>> return 0;
>>>>>>
>>>>>> old_crtc_state = intel_atomic_get_old_crtc_state(state, crtc);
>>>>>> - new_crtc_state = intel_atomic_get_new_crtc_state(state, crtc);
>>>>>>
>>>>>> return intel_plane_atomic_check_with_state(old_crtc_state,
>>>>>> new_crtc_state,
>>>>>> diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.h b/drivers/gpu/drm/i915/display/intel_atomic_plane.h
>>>>>> index 59dd1fbb02ea..c2a1e7c86e6c 100644
>>>>>> --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.h
>>>>>> +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.h
>>>>>> @@ -23,7 +23,8 @@ unsigned int intel_plane_pixel_rate(const struct intel_crtc_state *crtc_state,
>>>>>>
>>>>>> unsigned int intel_plane_data_rate(const struct intel_crtc_state *crtc_state,
>>>>>> const struct intel_plane_state *plane_state);
>>>>>> -void intel_plane_copy_uapi_to_hw_state(struct intel_plane_state *plane_state,
>>>>>> +void intel_plane_copy_uapi_to_hw_state(const struct intel_crtc_state *crtc_state,
>>>>>> + struct intel_plane_state *plane_state,
>>>>>> const struct intel_plane_state *from_plane_state);
>>>>>> void intel_update_plane(struct intel_plane *plane,
>>>>>> const struct intel_crtc_state *crtc_state,
>>>>>> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
>>>>>> index c0715a3ea47b..579cccc1fd91 100644
>>>>>> --- a/drivers/gpu/drm/i915/display/intel_display.c
>>>>>> +++ b/drivers/gpu/drm/i915/display/intel_display.c
>>>>>> @@ -3718,7 +3718,7 @@ intel_find_initial_plane_obj(struct intel_crtc *intel_crtc,
>>>>>> drm_framebuffer_get(fb);
>>>>>>
>>>>>> plane_state->crtc = &intel_crtc->base;
>>>>>> - intel_plane_copy_uapi_to_hw_state(intel_state, intel_state);
>>>>>> + intel_plane_copy_uapi_to_hw_state(crtc_state, intel_state, intel_state);
>>>>>>
>>>>>> intel_frontbuffer_flush(to_intel_frontbuffer(fb), ORIGIN_DIRTYFB);
>>>>>>
>>>>>> @@ -12801,26 +12801,180 @@ static bool check_single_encoder_cloning(struct intel_atomic_state *state,
>>>>>> return true;
>>>>>> }
>>>>>>
>>>>>> +static int icl_unset_bigjoiner_plane_links(struct intel_atomic_state *state,
>>>>>> + struct intel_crtc_state *new_crtc_state)
>>>>>> +{
>>>>>> + struct intel_crtc *crtc = to_intel_crtc(new_crtc_state->uapi.crtc);
>>>>>> + struct intel_plane *plane;
>>>>>> +
>>>>>> + /*
>>>>>> + * Teardown the old bigjoiner plane mappings.
>>>>>> + */
>>>>>> + for_each_intel_plane_on_crtc(crtc->base.dev, crtc, plane) {
>>>>>> + struct intel_plane_state *plane_state, *other_plane_state;
>>>>>> + struct intel_plane *other_plane;
>>>>>> +
>>>>>> + plane_state = intel_atomic_get_plane_state(state, plane);
>>>>>> + if (IS_ERR(plane_state))
>>>>>> + return PTR_ERR(plane_state);
>>>>>> +
>>>>>> + other_plane = plane_state->bigjoiner_plane;
>>>>>> + if (!other_plane)
>>>>>> + continue;
>>>>>> +
>>>>>> + plane_state->bigjoiner_plane = NULL;
>>>>>> + plane_state->bigjoiner_slave = false;
>>>>>> +
>>>>>> + other_plane_state = intel_atomic_get_plane_state(state, other_plane);
>>>>>> + if (IS_ERR(other_plane_state))
>>>>>> + return PTR_ERR(other_plane_state);
>>>>>> + other_plane_state->bigjoiner_plane = NULL;
>>>>>> + other_plane_state->bigjoiner_slave = false;
>>>>> Why would we even need this bigjoiner stuff in the planes? AFAICS about
>>>>> the only thing we should need is someting like
>>>>>
>>>>> for_each_plane_on_master()
>>>>> add_same_plane_on_slave()
>>>>>
>>>>> somewhere before we do the plane->check() stuff. I guess start
>>>>> of intel_atomic_check_planes() could be the right spot.
>>>>>
>>>> Yes may be but honestly I leave this optimization/change to the original
>>>> author Maarten or you as a follow up
>>> I don't want to see several hundred lines of totally uneccessary code
>>> added. If it's buggy (which it may very well be because it's too big to
>>> review properly) we are going to have to revert it anyway. If anything
>>> else has changed in the same code the revertr is going to be a huge
>>> pain.
>>>
>> This entire patch just does the linking of planes and there is no
>> unnecessary code.
> Yes there is. Each plane should have a proper hw state so there
> should be absolutely no need for this linking stuff.
>
>> Also since I am just a carrier of this code and not
>> the original author I dont know how this can be simplified
>> without breaking the functionality.
> You don't understand the code, I don't understand the code. How do
> you suggest we can merge this? If there is any problem with the code
> we have no choice but a full revert.
Hey,
There's good reason to link the planes. The reason is similar to linking Y and CbCr planes.
You're completely correct that hardware programming doesn't need the linking, and planes are
in theory standalone.
But there's also atomic. The lifetime of plane_state on the bigjoiner slave plane must be
linked to the lifetime of the master plane. Otherwise if you do a commit on the master plane,
you may get a use after free on slave. To prevent this I just linked them together. :)
~Maarten
~Maarten
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/intel-gfx/attachments/20201028/d858bd53/attachment.htm>
More information about the Intel-gfx
mailing list