On 5/4/2018 6:27 PM, Andrey Grodzovsky wrote:
>
>
> On 05/03/2018 02:11 PM, Harry Wentland wrote:
>> On 2018-05-03 04:00 AM, S, Shirish wrote:
>>>
>>> On 5/2/2018 7:21 PM, Harry Wentland wrote:
>>>> On 2018-04-27 06:27 AM, Shirish S wrote:
>>>>> This patch is in continuation to the
>>>>> "843e3c7 drm/amd/display: defer modeset check in
>>>>> dm_update_planes_state"
>>>>> where we started to eliminate the dependency on
>>>>> DRM_MODE_ATOMIC_ALLOW_MODESET to be set by the user space,
>>>>> which as such is not mandatory.
>>>>>
>>>>> After deferring, this patch eliminates the dependency on the flag
>>>>> for overlay planes.
>>>>>
>>>> Apologies for the late response. I had to think about this patch
>>>> for a long time since I'm not quite comfortable with it.
>>>>
>>>>> This has to be done in stages as its a pretty complex and requires
>>>>> thorough
>>>>> testing before we free primary planes as well from dependency on
>>>>> modeset
>>>>> flag.
>>>>>
>>>>> Signed-off-by: Shirish S <shirish.s at amd.com>
>>>>> ---
>>>>> Â Â drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 8 +++++---
>>>>> Â Â 1 file changed, 5 insertions(+), 3 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 1a63c04..87b661d 100644
>>>>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>>>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>>>> @@ -4174,7 +4174,7 @@ static void amdgpu_dm_commit_planes(struct
>>>>> drm_atomic_state *state,
>>>>> Â Â Â Â Â Â Â Â Â Â }
>>>>> spin_unlock_irqrestore(&crtc->dev->event_lock, flags);
>>>>> Â Â -Â Â Â Â Â Â Â if (!pflip_needed) {
>>>>> +Â Â Â Â Â Â Â if (!pflip_needed || plane->type ==
>>>>> DRM_PLANE_TYPE_OVERLAY) {
>>>> Does this mean that whenever we have an overlay plane we won't do
>>>> amdgpu_dm_do_flip but commit_planes_to_stream instead? Is this
>>>> really the behavior we want?
>>>>
>>>> commit_planes_to_stream was intended to program a new surface on a
>>>> modeset whereas amdgpu_dm_do_flip was intended for pageflips.
>>> Need of "modeset" flag to program new surface is what we want to fix
>>> in this patch for underlay plane and in next stages, fix
>>> manifestations caused by this approach as and when seen.
>>> Since the user space doesn't send modeset flag for new surface,
>>> hence to program it, this patch checks the plane type to construct
>>> planes_count before calling commit_planes_to_stream().
>>>
>> Looking at the allow_modeset flag was never quite right and we
>> anticipated having to rework this when having to deal with things
>> like multiple planes. What really has to happen is that we determine
>> the surface_update_type in atomic_check and then use that in
>> atomic_commit to either program the surface only (UPDATE_TYPE_FAST)
>> without having to lock all pipes or to lock all pipes (see
>> lock_and_validation_needed in amdgpu_dm_atomic_check) if we need to
>> reprogram mode (UPDATE_TYPE_FULL). I don't remember exactly what
>> UPDATE_TYPE_MED is used for.
>>
True, any suggestions on what needs to be check to decide if the surface
update has to be FAST?
Like plane_count, format or dc_state etc?
>> I don't feel comfortable taking a shortcut for DRM_PLANE_TYPE_OVERLAY
>> without first having a plan and patches for how to deal with the
>> above-mentioned.
>>
>> Bhawan and Andrey had a look at this before but it was never quite
>> ready. The work was non-trivial and potentially impacts lots of
>> configurations and scenarios if we don't get it right. If you're
>> curious you can look at this change (apologies to everyone else for
>> posting AMD-internal link): http://git.amd.com:8080/#/c/103931/11
>>
>>> Â Â If we use commit_planes_to_stream we end up losing things like the
>>> immediate_flip flag, as well as the wait for the right moment to
>>> program the flip that amdgpu_dm_do_flip does.
>>>
>>> Â From the code, amdgpu_dm_do_flip does what you mentioned only for
>>> primary plane and hence either way its not set for underlay.
>> The code wasn't designed with underlay in mind, so it will need work.
>>
>> Harry
>
> I support Harry's comments, we definitely need to strive to remove
> dependency on page_fleep needed flag, AFAIK we are the only ATOMIC KMS
> driver which makes a distinction between page fleep and other
> surface updates, but it's better to sit and create a general plan of
> how to address it for all type of planes instead of patching for
> overlay only.
>
> Andrey
>
Is anybody working on removing the dependency on page_flip_needed flag ?
Since am working on stoney for chrome os, i have limited visibility or
knowledge of its implications on other asics,
hence to avoid regressions to other users i patched only the overlay
path of it.
Some one who knows its inter-dependency on dc would be right to do it.
If you think its a long shot please re-consider my proposal of doing it
in stages, i.e., removing overlay planes from dependency and then the
primary since cursor updates is a but tricky and tied to primary plane.
Regards,
Shirish S
>>
>>> Regards,
>>> Shirish S
>>>> Â Â Even more importantly we won't wait for fences
>>>> (reservation_object_wait_timeout_rcu).
>>>>
>>>> Harry
>>>>
>>>>> WARN_ON(!dm_new_plane_state->dc_state);
>>>>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â plane_states_constructed[planes_count] =
>>>>> dm_new_plane_state->dc_state;
>>>>> @@ -4884,7 +4884,8 @@ static int dm_update_planes_state(struct dc
>>>>> *dc,
>>>>> Â Â Â Â Â Â Â Â Â Â Â /* Remove any changed/removed planes */
>>>>> Â Â Â Â Â Â Â Â Â Â if (!enable) {
>>>>> -Â Â Â Â Â Â Â Â Â Â Â if (pflip_needed)
>>>>> +Â Â Â Â Â Â Â Â Â Â Â if (pflip_needed &&
>>>>> +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â plane && plane->type != DRM_PLANE_TYPE_OVERLAY)
>>>>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â continue;
>>>>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â if (!old_plane_crtc)
>>>>> @@ -4931,7 +4932,8 @@ static int dm_update_planes_state(struct dc
>>>>> *dc,
>>>>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â if (!dm_new_crtc_state->stream)
>>>>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â continue;
>>>>> Â Â -Â Â Â Â Â Â Â Â Â Â Â if (pflip_needed)
>>>>> +Â Â Â Â Â Â Â Â Â Â Â if (pflip_needed &&
>>>>> +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â plane && plane->type != DRM_PLANE_TYPE_OVERLAY)
>>>>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â continue;
>>>>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â WARN_ON(dm_new_plane_state->dc_state);
>>>>>
>
--
Regards,
Shirish S
