[PATCH 2/4] drm/msm/dp: remove redundant ST_DISPLAY_OFF checks in msm_dp_bridge_atomic_enable()

[Abhinav Kumar]

On 12/23/2024 10:32 PM, Dmitry Baryshkov wrote:
> On Wed, Dec 04, 2024 at 12:32:55PM +0200, Dmitry Baryshkov wrote:
>> On Tue, Dec 03, 2024 at 07:24:46PM -0800, Abhinav Kumar wrote:
>>>
>>>
>>> On 12/3/2024 5:53 AM, Dmitry Baryshkov wrote:
>>>> On Mon, Dec 02, 2024 at 04:39:01PM -0800, Abhinav Kumar wrote:
>>>>> The checks in msm_dp_display_prepare() for making sure that we are in
>>>>> ST_DISPLAY_OFF OR ST_MAINLINK_READY seem redundant.
>>>>>
>>>>> DRM fwk shall not issue any commits if state is not ST_MAINLINK_READY as
>>>>> msm_dp's atomic_check callback returns a failure if state is not ST_MAINLINK_READY.
>>>>
>>>> Can the state change between atomic_check() and atomic_commit()?
>>>>
>>>
>>> Good question.
>>>
>>> I cannot deny that such a possibility does exist.
>>>
>>>  From what I can see in the state machine today, the only possibility I can
>>> think of here is if a user very quickly removes the cable as soon as they
>>> connect the cable like so fast that the connect was not yet processed before
>>> disconnect.
>>
>> If the cable has electrical issues, it is possible even w/o user
>> intervention.
> 
> And the possible desynchronisation between DRM atomic states and HPD
> state machine brings back the topic: can we get rid of the state
> machine instead of trying to fix it? I'd rather see a patchset that
> removes it completely. The link training should go to the atomic_enable,
> etc, etc. Please? Pretty please? I'd rather see imperfect solution which
> has possible issues with some of the dongles (as they can be fixed later
> on) than having the imperfect HPD state machine which is known to break
> DRM framework expectations.
> 

Sorry for the delayed response.

The activity to move the link training to atomic_enable and to get rid 
of the state machine has started.

But, it is being done on top of this change only because this series 
actually gets rid of some states.

I will address the remaining comment on this and repost the next 
revision. I would suggest that if the state machine removal happens 
smooth, we can squash this series that with that one and post it 
together again and merge them together.

But if it takes longer than expected,  I think we should be open to 
merging this one and MST (with the comments addressed ofcourse) and the 
state machine removal goes on top.

Either way, this series is only helping the cause of getting rid of some 
of the states.

Thanks

Abhinav
>>
>>>
>>> Similarly, if an irq_hpd fires after atomic_check but before
>>> atomic_enable(), and moreover if we hit the sink_count == 0 case in
>>> msm_dp_display_handle_port_status_changed() during this irq_hpd,
>>>
>>> In both these cases, then we will transition to ST_DISCONNECT_PENDING state.
>>>
>>> Without this change, we would have bailed out in the ST_DISCONNECT_PENDING
>>> case.
>>>
>>> But other than this, I cannot atleast think of a case where a different
>>> state transition can happen between atomic_check() and atomic_commit()
>>> because for other transitions, I think we should be still okay.
>>>
>>> But this is purely based on theoretical observation and hypothesis.
>>>
>>> Is it better to add a check to bail out in the DISCONNECT_PENDING case?
>>
>> I think so, please.
>>
>>>
>>> OR document this as "To-do: Need to bail out if DISCONNECT_PENDING" because
>>> even if I add this check, I dont know if can make sure this can be validated
>>> as the check could never hit.
>>>
>>>
>>>>>
>>>>> For the ST_DISPLAY_OFF check, its mainly to guard against a scenario that
>>>>> there is an atomic_enable() without a prior atomic_disable() which once again
>>>>> should not really happen.
>>>>>
>>>>> To simplify the code, get rid of these checks.
>>>>>
>>>>> Signed-off-by: Abhinav Kumar <quic_abhinavk at quicinc.com>
>>>>> ---
>>>>>    drivers/gpu/drm/msm/dp/dp_display.c | 6 ------
>>>>>    1 file changed, 6 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
>>>>> index 992184cc17e4..614fff09e5f2 100644
>>>>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
>>>>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
>>>>> @@ -1513,12 +1513,6 @@ void msm_dp_bridge_atomic_enable(struct drm_bridge *drm_bridge,
>>>>>    		return;
>>>>>    	}
>>>>> -	state = msm_dp_display->hpd_state;
>>>>> -	if (state != ST_DISPLAY_OFF && state != ST_MAINLINK_READY) {
>>>>> -		mutex_unlock(&msm_dp_display->event_mutex);
>>>>> -		return;
>>>>> -	}
>>>>> -
>>>>>    	rc = msm_dp_display_set_mode(dp, &msm_dp_display->msm_dp_mode);
>>>>>    	if (rc) {
>>>>>    		DRM_ERROR("Failed to perform a mode set, rc=%d\n", rc);
>>>>>
>>>>> -- 
>>>>> 2.34.1
>>>>>
>>>>
>>
>> -- 
>> With best wishes
>> Dmitry
>