Intel-GFX Archive on lore.kernel.org
 help / color / mirror / Atom feed
From: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
To: "Ville Syrjälä" <ville.syrjala@linux.intel.com>,
	"Navare, Manasi" <manasi.d.navare@intel.com>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [Intel-gfx] [PATCH v11 10/12] drm/i915: Link planes in a bigjoiner configuration, v3.
Date: Wed, 28 Oct 2020 13:26:27 +0100	[thread overview]
Message-ID: <dec38bb5-c6a2-18dc-684c-92564e630451@linux.intel.com> (raw)
In-Reply-To: <20201027191150.GP6112@intel.com>


[-- Attachment #1.1: Type: text/plain, Size: 11528 bytes --]

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@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@linux.intel.com>
>>>>>> Signed-off-by: Manasi Navare <manasi.d.navare@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


[-- Attachment #1.2: Type: text/html, Size: 11729 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

  reply	other threads:[~2020-10-28 12:26 UTC|newest]

Thread overview: 47+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-10-22  5:42 [Intel-gfx] [PATCH v11 00/12] Big joiner enabling Manasi Navare
2020-10-22  5:42 ` [Intel-gfx] [PATCH v11 01/12] HAX to make DSC work on the icelake test system Manasi Navare
2020-10-22  5:42 ` [Intel-gfx] [PATCH v11 02/12] drm/i915: Add hw.pipe_mode to allow bigjoiner pipe/transcoder split Manasi Navare
2020-10-22  5:42 ` [Intel-gfx] [PATCH v11 03/12] drm/i915/dp: Some reshuffling in mode_valid as prep for bigjoiner modes Manasi Navare
2020-10-23 17:17   ` Ville Syrjälä
2020-10-22  5:42 ` [Intel-gfx] [PATCH v11 04/12] drm/i915/dp: Allow big joiner modes in intel_dp_mode_valid(), v3 Manasi Navare
2020-10-23 17:32   ` Ville Syrjälä
2020-10-23 18:30     ` Navare, Manasi
2020-10-23 18:44       ` Ville Syrjälä
2020-10-26 23:47         ` Navare, Manasi
2020-10-27  5:50   ` [Intel-gfx] [PATCH v12 " Manasi Navare
2020-10-22  5:42 ` [Intel-gfx] [PATCH v11 05/12] drm/i915/dp: Prep for bigjoiner atomic check Manasi Navare
2020-10-27  5:50   ` [Intel-gfx] [PATCH v12 " Manasi Navare
2020-10-22  5:42 ` [Intel-gfx] [PATCH v11 06/12] drm/i915: Try to make bigjoiner work in " Manasi Navare
2020-10-23 17:42   ` Ville Syrjälä
2020-10-23 18:13     ` Navare, Manasi
2020-10-23 18:30       ` Ville Syrjälä
2020-10-27  5:50   ` [Intel-gfx] [PATCH v12 " Manasi Navare
2020-10-22  5:42 ` [Intel-gfx] [PATCH v11 07/12] drm/i915/dp: Modify VDSC helpers to configure DSC for Bigjoiner slave Manasi Navare
2020-10-26 21:56   ` Navare, Manasi
2020-10-22  5:42 ` [Intel-gfx] [PATCH v11 08/12] drm/i915/dp: Master/Slave enable/disable sequence for bigjoiner Manasi Navare
2020-10-23  7:57   ` Dan Carpenter
2020-10-26 21:57   ` Navare, Manasi
2020-10-27  5:50   ` [Intel-gfx] [PATCH v12 " Manasi Navare
2020-10-22  5:42 ` [Intel-gfx] [PATCH v11 09/12] drm/i915: HW state readout for Bigjoiner case Manasi Navare
2020-10-23 18:00   ` Ville Syrjälä
2020-10-26 22:33     ` Navare, Manasi
2020-10-27 13:39       ` Ville Syrjälä
2020-10-27 18:11         ` Navare, Manasi
2020-10-26 22:29   ` Navare, Manasi
2020-10-22  5:42 ` [Intel-gfx] [PATCH v11 10/12] drm/i915: Link planes in a bigjoiner configuration, v3 Manasi Navare
2020-10-26 20:18   ` Ville Syrjälä
2020-10-26 22:34     ` Navare, Manasi
2020-10-26 22:41     ` Navare, Manasi
2020-10-27 13:42       ` Ville Syrjälä
2020-10-27 18:19         ` Navare, Manasi
2020-10-27 19:11           ` Ville Syrjälä
2020-10-28 12:26             ` Maarten Lankhorst [this message]
2020-10-28 13:04               ` Ville Syrjälä
2020-10-28 22:15                 ` Navare, Manasi
2020-10-22  5:42 ` [Intel-gfx] [PATCH v11 11/12] drm/i915: Add bigjoiner aware plane clipping checks Manasi Navare
2020-10-22  5:42 ` [Intel-gfx] [PATCH v11 12/12] drm/i915: Add debugfs dumping for bigjoiner, v3 Manasi Navare
2020-10-22  6:01 ` [Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for Big joiner enabling Patchwork
2020-10-22  6:03 ` [Intel-gfx] ✗ Fi.CI.SPARSE: " Patchwork
2020-10-22  6:26 ` [Intel-gfx] ✓ Fi.CI.BAT: success " Patchwork
2020-10-22  8:09 ` [Intel-gfx] ✓ Fi.CI.IGT: " Patchwork
2020-10-28  0:28 ` [Intel-gfx] ✗ Fi.CI.BUILD: failure for Big joiner enabling (rev5) Patchwork

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=dec38bb5-c6a2-18dc-684c-92564e630451@linux.intel.com \
    --to=maarten.lankhorst@linux.intel.com \
    --cc=intel-gfx@lists.freedesktop.org \
    --cc=manasi.d.navare@intel.com \
    --cc=ville.syrjala@linux.intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox