Quoting khsieh@xxxxxxxxxxxxxx (2020-10-05 11:02:10) > >> + dp_del_event(dp_display, EV_DISCONNECT_PENDING_TIMEOUT); > >> + > >> dp_display_disable(dp_display, 0); > >> > >> rc = dp_display_unprepare(dp); > >> if (rc) > >> DRM_ERROR("DP display unprepare failed, rc=%d\n", rc); > >> > >> - dp_del_event(dp_display, EV_DISCONNECT_PENDING_TIMEOUT); > >> - > >> state = atomic_read(&dp_display->hpd_state); > >> if (state == ST_DISCONNECT_PENDING) { > > > > I don't understand the atomic nature of this hpd_state variable. Why is > > it an atomic variable? Is taking a spinlock bad? What is to prevent the > > atomic read here to not be interrupted and then this if condition check > > be invalid because the variable has been updated somewhere else? > hpd_state variable updated by multiple threads. however it was protected > by mutex. > in theory, it should also work as u32. since it was declared as atomic > from beginning > and it does not cause any negative effects, can we keep it as it is? > It does cause negative effects by generating worse code for something that is already protected from concurrency by a mutex. Can we make it an enum and name the enum and then add a comment indicating that the 'event_mutex' lock protects this variable? _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel