| Message ID | 20241202-hpd_display_off-v1-2-8d0551847753@quicinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | drm/msm/dp: ST_DISPLAY_OFF hpd cleanup | expand |
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. > > > > > 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@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
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@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 >
On Thu, May 22, 2025 at 09:21:00PM -0700, Abhinav Kumar wrote: > > > 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. No-no-no. State machine removal _must_ come before MST. Pretty much like within the series the Fixes patches should come first. Otherwise it's impossible to backport fixes to earlier series. Not to mention that MST is a complicated code per se. Most likely it will take several revisions to merge it. > > Either way, this series is only helping the cause of getting rid of some of > the states. And possibly introduces new issues if the state changes between atomic_check() and atomic_commit(), if I remember correctly. I'm really asking to stop trying to do small fixes here and there and rewrite this piece of code. I really don't want to see actual MST implementation before HPD gets fixed. > > 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@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 > > >
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);
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. 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@quicinc.com> --- drivers/gpu/drm/msm/dp/dp_display.c | 6 ------ 1 file changed, 6 deletions(-)