| Message ID | 20241202-hpd_display_off-v1-2-8d0551847753@quicinc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/msm/dp: ST_DISPLAY_OFF hpd cleanup | expand |
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()? > > 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 >
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. 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? 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 >> >
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. > > 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 > > > > >
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
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(-)