On 3/20/24 02:53, Tomi Valkeinen wrote: > On 20/03/2024 00:51, Sean Anderson wrote: >> Retraining the link can take a while, and might involve waiting for >> DPCD reads/writes to complete. This is inappropriate for an IRQ handler. >> Just schedule this work for later completion. This is racy, but will be >> fixed in the next commit. > > You should add the locks first, and use them here, rather than first > adding a buggy commit and fixing it in the next one. I didn't think I could add the locks first since I only noticed the IRQ was threaded right before sending out this series. So yeah, we could add locking, add the workqueue, and then unthread the IRQ. >> Signed-off-by: Sean Anderson <sean.anderson@xxxxxxxxx> >> --- >> Actually, on second look this IRQ is threaded. So why do we have a >> workqueue for HPD events? Maybe we should make it unthreaded? > > Indeed, there's not much work being done in the IRQ handler. I don't know why it's threaded. > > We could move the queued work to be inside the threaded irq handler, > but with a quick look, the HPD work has lines like "msleep(100)" (and > that's inside a for loop...), which is probably not a good thing to do > even in threaded irq handler. > > Although I'm not sure if that code is good to have anywhere. Why do we > even have such code in the HPD work path... We already got the HPD > interrupt. What does "It takes some delay (ex, 100 ~ 500 msec) to get > the HPD signal with some monitors" even mean... The documentation for this bit is | HPD_STATE 0 ro 0x0 Contains the raw state of the HPD pin on the DisplayPort connector. So I think the idea is to perform some debouncing. > Would it be possible to clean up the work funcs a bit (I haven't > looked a the new work func yet), to remove the worst extra sleeps, and > just do all that inside the threaded irq handler? Probably not, since a HPD IRQ results in link retraining, which can take a while. > Do we need to handle interrupts while either delayed work is being done? Probably not. > If we do need a delayed work, would just one work be enough which > handles both HPD_EVENT and HPD_IRQ, instead of two? Maybe, but then we need to determine which pending events we need to handle. I think since we have only two events it will be easier to just have separate workqueues. --Sean
