| Message ID | 20241202-hpd_display_off-v1-2-8d0551847753@quicinc.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| 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 > > > > >
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(-)