On Wed, Oct 28, 2020 at 03:04:37PM +0200, Ville Syrjälä wrote:
> On Wed, Oct 28, 2020 at 01:26:27PM +0100, Maarten Lankhorst wrote:
> > 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.lankho...@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.lankho...@linux.intel.com>
> > >>>>>> Signed-off-by: Manasi Navare <manasi.d.nav...@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.
>
> There we actually have to link them in some fashion because that's what
> the hw requires. But with the uapi vs. hw state split we should be able
> to make a proper state copy there too, and thus we could get rid of the
> magic "let's use the other plane's state during commit" stuff.
>
> > 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. :)
>
> There should be nothing in the slave's plane state that references the
> master's state. Whatever pointers we have there (fb/gamma/etc.) should
> be refcounted. So can't immediately think where any uaf would come from.
>
> That said. Looks like watermarks are a bit of a mess again. Time to
> finally get rid of that intel_atomic_crtc_state_for_each_plane_state()
> I think...
>
So can we move forward with the plane linking as of now or would @Maarten have
some BW to change this?
Manasi
> --
> Ville Syrjälä
> Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
