Re: [PATCH] drm/amd/display: Add fast path for cursor plane updates

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 12/5/18 4:01 PM, Kazlauskas, Nicholas wrote:
> On 2018-12-05 3:44 p.m., Grodzovsky, Andrey wrote:
>>
>>
>> On 12/05/2018 03:42 PM, Kazlauskas, Nicholas wrote:
>>> On 2018-12-05 3:26 p.m., Grodzovsky, Andrey wrote:
>>>>
>>>> On 12/05/2018 02:59 PM, Nicholas Kazlauskas wrote:
>>>>> [Why]
>>>>> Legacy cursor plane updates from drm helpers go through the full
>>>>> atomic codepath. A high volume of cursor updates through this slow
>>>>> code path can cause subsequent page-flips to skip vblank intervals
>>>>> since each individual update is slow.
>>>>>
>>>>> This problem is particularly noticeable for the compton compositor.
>>>>>
>>>>> [How]
>>>>> A fast path for cursor plane updates is added by using DRM asynchronous
>>>>> commit support provided by async_check and async_update. These don't do
>>>>> a full state/flip_done dependency stall and they don't block other
>>>>> commit work.
>>>>>
>>>>> However, DC still expects itself to be single-threaded for anything
>>>>> that can issue register writes. Screen corruption or hangs can occur
>>>>> if write sequences overlap. Every call that potentially perform
>>>>> register writes needs to be guarded for asynchronous updates to work.
>>>>> The dc_lock mutex was added for this.
>>>> As far as page flip related register writes concerned this I think this
>>>> was never an issue - we always
>>>> supported fully concurrent page flips on different CRTCs meaning writing
>>>> surface address concurrently
>>>> on different pipes. So Are you sure you  can't do cursor update
>>>> concurrently against at least page flips ?
>>>> I am not sure about full updates.
>>>>
>>>> Andrey
>>> I think this was true before we started locking the pipes around cursor
>>> updates (and probably only for dce). The problem that occur here is
>>> writing to the lock register from multiple threads at the same time.
>>>
>>> In general I think the "contract" between dm/dc now is that anything
>>> from dc that does register write sequences can't be called from multiple
>>> threads.
>>>
>>> Nicholas Kazlauskas
>>
>> Ok, do note that you also serialize all the page flips now which looks
>> unneeded - maybe consider r/w lock to at least avoid that.
>>
>> Andrey
> 
> Yeah, they definitely shouldn't be happening at the same time as the
> cursor calls.
> 
> I'd need to look into whether it's okay now to do concurrent writes from
> the stream update functions though, but I'd imagine it's not something
> we'd want to be doing. A r/w lock would work if we could though.
> 
> Nicholas Kazlauskas

A reader/writer lock wouldn't help here, actually.

Fast updates (and page flips in particular) need to lock the pipe as 
well, see: commit_planes_for_stream.

Nicholas Kazlauskas

> 
>>
>>>
>>>>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=106175
>>>>>
>>>>> Cc: Leo Li <sunpeng.li@xxxxxxx>
>>>>> Cc: Harry Wentland <harry.wentland@xxxxxxx>
>>>>> Signed-off-by: Nicholas Kazlauskas <nicholas.kazlauskas@xxxxxxx>
>>>>> ---
>>>>>       .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 67 ++++++++++++++++++-
>>>>>       .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h |  8 +++
>>>>>       2 files changed, 73 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>>>> index 23d61570df17..4df14a50a8ac 100644
>>>>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>>>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>>>> @@ -57,6 +57,7 @@
>>>>>       
>>>>>       #include <drm/drmP.h>
>>>>>       #include <drm/drm_atomic.h>
>>>>> +#include <drm/drm_atomic_uapi.h>
>>>>>       #include <drm/drm_atomic_helper.h>
>>>>>       #include <drm/drm_dp_mst_helper.h>
>>>>>       #include <drm/drm_fb_helper.h>
>>>>> @@ -133,6 +134,8 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state);
>>>>>       static int amdgpu_dm_atomic_check(struct drm_device *dev,
>>>>>       				  struct drm_atomic_state *state);
>>>>>       
>>>>> +static void handle_cursor_update(struct drm_plane *plane,
>>>>> +				 struct drm_plane_state *old_plane_state);
>>>>>       
>>>>>       
>>>>>       
>>>>> @@ -402,6 +405,8 @@ static int amdgpu_dm_init(struct amdgpu_device *adev)
>>>>>       	/* Zero all the fields */
>>>>>       	memset(&init_data, 0, sizeof(init_data));
>>>>>       
>>>>> +	mutex_init(&adev->dm.dc_lock);
>>>>> +
>>>>>       	if(amdgpu_dm_irq_init(adev)) {
>>>>>       		DRM_ERROR("amdgpu: failed to initialize DM IRQ support.\n");
>>>>>       		goto error;
>>>>> @@ -516,6 +521,9 @@ static void amdgpu_dm_fini(struct amdgpu_device *adev)
>>>>>       	/* DC Destroy TODO: Replace destroy DAL */
>>>>>       	if (adev->dm.dc)
>>>>>       		dc_destroy(&adev->dm.dc);
>>>>> +
>>>>> +	mutex_destroy(&adev->dm.dc_lock);
>>>>> +
>>>>>       	return;
>>>>>       }
>>>>>       
>>>>> @@ -3615,10 +3623,43 @@ static int dm_plane_atomic_check(struct drm_plane *plane,
>>>>>       	return -EINVAL;
>>>>>       }
>>>>>       
>>>>> +static int dm_plane_atomic_async_check(struct drm_plane *plane,
>>>>> +				       struct drm_plane_state *new_plane_state)
>>>>> +{
>>>>> +	/* Only support async updates on cursor planes. */
>>>>> +	if (plane->type != DRM_PLANE_TYPE_CURSOR)
>>>>> +		return -EINVAL;
>>>>> +
>>>>> +	return 0;
>>>>> +}
>>>>> +
>>>>> +static void dm_plane_atomic_async_update(struct drm_plane *plane,
>>>>> +					 struct drm_plane_state *new_state)
>>>>> +{
>>>>> +	struct drm_plane_state *old_state =
>>>>> +		drm_atomic_get_old_plane_state(new_state->state, plane);
>>>>> +
>>>>> +	if (plane->state->fb != new_state->fb)
>>>>> +		drm_atomic_set_fb_for_plane(plane->state, new_state->fb);
>>>>> +
>>>>> +	plane->state->src_x = new_state->src_x;
>>>>> +	plane->state->src_y = new_state->src_y;
>>>>> +	plane->state->src_w = new_state->src_w;
>>>>> +	plane->state->src_h = new_state->src_h;
>>>>> +	plane->state->crtc_x = new_state->crtc_x;
>>>>> +	plane->state->crtc_y = new_state->crtc_y;
>>>>> +	plane->state->crtc_w = new_state->crtc_w;
>>>>> +	plane->state->crtc_h = new_state->crtc_h;
>>>>> +
>>>>> +	handle_cursor_update(plane, old_state);
>>>>> +}
>>>>> +
>>>>>       static const struct drm_plane_helper_funcs dm_plane_helper_funcs = {
>>>>>       	.prepare_fb = dm_plane_helper_prepare_fb,
>>>>>       	.cleanup_fb = dm_plane_helper_cleanup_fb,
>>>>>       	.atomic_check = dm_plane_atomic_check,
>>>>> +	.atomic_async_check = dm_plane_atomic_async_check,
>>>>> +	.atomic_async_update = dm_plane_atomic_async_update
>>>>>       };
>>>>>       
>>>>>       /*
>>>>> @@ -4307,6 +4348,7 @@ static int get_cursor_position(struct drm_plane *plane, struct drm_crtc *crtc,
>>>>>       static void handle_cursor_update(struct drm_plane *plane,
>>>>>       				 struct drm_plane_state *old_plane_state)
>>>>>       {
>>>>> +	struct amdgpu_device *adev = plane->dev->dev_private;
>>>>>       	struct amdgpu_framebuffer *afb = to_amdgpu_framebuffer(plane->state->fb);
>>>>>       	struct drm_crtc *crtc = afb ? plane->state->crtc : old_plane_state->crtc;
>>>>>       	struct dm_crtc_state *crtc_state = crtc ? to_dm_crtc_state(crtc->state) : NULL;
>>>>> @@ -4331,9 +4373,12 @@ static void handle_cursor_update(struct drm_plane *plane,
>>>>>       
>>>>>       	if (!position.enable) {
>>>>>       		/* turn off cursor */
>>>>> -		if (crtc_state && crtc_state->stream)
>>>>> +		if (crtc_state && crtc_state->stream) {
>>>>> +			mutex_lock(&adev->dm.dc_lock);
>>>>>       			dc_stream_set_cursor_position(crtc_state->stream,
>>>>>       						      &position);
>>>>> +			mutex_unlock(&adev->dm.dc_lock);
>>>>> +		}
>>>>>       		return;
>>>>>       	}
>>>>>       
>>>>> @@ -4351,6 +4396,7 @@ static void handle_cursor_update(struct drm_plane *plane,
>>>>>       	attributes.pitch = attributes.width;
>>>>>       
>>>>>       	if (crtc_state->stream) {
>>>>> +		mutex_lock(&adev->dm.dc_lock);
>>>>>       		if (!dc_stream_set_cursor_attributes(crtc_state->stream,
>>>>>       							 &attributes))
>>>>>       			DRM_ERROR("DC failed to set cursor attributes\n");
>>>>> @@ -4358,6 +4404,7 @@ static void handle_cursor_update(struct drm_plane *plane,
>>>>>       		if (!dc_stream_set_cursor_position(crtc_state->stream,
>>>>>       						   &position))
>>>>>       			DRM_ERROR("DC failed to set cursor position\n");
>>>>> +		mutex_unlock(&adev->dm.dc_lock);
>>>>>       	}
>>>>>       }
>>>>>       
>>>>> @@ -4573,6 +4620,7 @@ static void amdgpu_dm_do_flip(struct drm_crtc *crtc,
>>>>>       				&acrtc_state->stream->vrr_infopacket;
>>>>>       	}
>>>>>       
>>>>> +	mutex_lock(&adev->dm.dc_lock);
>>>>>       	dc_commit_updates_for_stream(adev->dm.dc,
>>>>>       					     surface_updates,
>>>>>       					     1,
>>>>> @@ -4580,6 +4628,7 @@ static void amdgpu_dm_do_flip(struct drm_crtc *crtc,
>>>>>       					     &stream_update,
>>>>>       					     &surface_updates->surface,
>>>>>       					     state);
>>>>> +	mutex_unlock(&adev->dm.dc_lock);
>>>>>       
>>>>>       	DRM_DEBUG_DRIVER("%s Flipping to hi: 0x%x, low: 0x%x \n",
>>>>>       			 __func__,
>>>>> @@ -4594,6 +4643,7 @@ static void amdgpu_dm_do_flip(struct drm_crtc *crtc,
>>>>>        * with a dc_plane_state and follow the atomic model a bit more closely here.
>>>>>        */
>>>>>       static bool commit_planes_to_stream(
>>>>> +		struct amdgpu_display_manager *dm,
>>>>>       		struct dc *dc,
>>>>>       		struct dc_plane_state **plane_states,
>>>>>       		uint8_t new_plane_count,
>>>>> @@ -4670,11 +4720,13 @@ static bool commit_planes_to_stream(
>>>>>       		updates[i].scaling_info = &scaling_info[i];
>>>>>       	}
>>>>>       
>>>>> +	mutex_lock(&dm->dc_lock);
>>>>>       	dc_commit_updates_for_stream(
>>>>>       			dc,
>>>>>       			updates,
>>>>>       			new_plane_count,
>>>>>       			dc_stream, stream_update, plane_states, state);
>>>>> +	mutex_unlock(&dm->dc_lock);
>>>>>       
>>>>>       	kfree(flip_addr);
>>>>>       	kfree(plane_info);
>>>>> @@ -4780,7 +4832,8 @@ static void amdgpu_dm_commit_planes(struct drm_atomic_state *state,
>>>>>       
>>>>>       		dc_stream_attach->abm_level = acrtc_state->abm_level;
>>>>>       
>>>>> -		if (false == commit_planes_to_stream(dm->dc,
>>>>> +		if (false == commit_planes_to_stream(dm,
>>>>> +							dm->dc,
>>>>>       							plane_states_constructed,
>>>>>       							planes_count,
>>>>>       							acrtc_state,
>>>>> @@ -4950,7 +5003,9 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state)
>>>>>       
>>>>>       	if (dc_state) {
>>>>>       		dm_enable_per_frame_crtc_master_sync(dc_state);
>>>>> +		mutex_lock(&dm->dc_lock);
>>>>>       		WARN_ON(!dc_commit_state(dm->dc, dc_state));
>>>>> +		mutex_unlock(&dm->dc_lock);
>>>>>       	}
>>>>>       
>>>>>       	for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) {
>>>>> @@ -5012,6 +5067,7 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state)
>>>>>       
>>>>>       		/*TODO How it works with MPO ?*/
>>>>>       		if (!commit_planes_to_stream(
>>>>> +				dm,
>>>>>       				dm->dc,
>>>>>       				status->plane_states,
>>>>>       				status->plane_count,
>>>>> @@ -5904,6 +5960,13 @@ static int amdgpu_dm_atomic_check(struct drm_device *dev,
>>>>>       			ret = -EINVAL;
>>>>>       			goto fail;
>>>>>       		}
>>>>> +	} else if (state->legacy_cursor_update) {
>>>>> +		/*
>>>>> +		 * This is a fast cursor update coming from the plane update
>>>>> +		 * helper, check if it can be done asynchronously for better
>>>>> +		 * performance.
>>>>> +		 */
>>>>> +		state->async_update = !drm_atomic_helper_async_check(dev, state);
>>>>>       	}
>>>>>       
>>>>>       	/* Must be success */
>>>>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
>>>>> index 4326dc256491..25bb91ee80ba 100644
>>>>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
>>>>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
>>>>> @@ -134,6 +134,14 @@ struct amdgpu_display_manager {
>>>>>       
>>>>>       	struct drm_modeset_lock atomic_obj_lock;
>>>>>       
>>>>> +	/**
>>>>> +	 * @dc_lock:
>>>>> +	 *
>>>>> +	 * Guards access to DC functions that can issue register write
>>>>> +	 * sequences.
>>>>> +	 */
>>>>> +	struct mutex dc_lock;
>>>>> +
>>>>>       	/**
>>>>>       	 * @irq_handler_list_low_tab:
>>>>>       	 *
>>
> 

_______________________________________________
amd-gfx mailing list
amd-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/amd-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux