Messages in this thread | | | Date | Thu, 21 Mar 2024 11:52:23 -0400 | Subject | Re: [PATCH v2 5/8] drm: zynqmp_dp: Don't retrain the link in our IRQ | From | Sean Anderson <> |
| |
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@linux.dev> >> --- >> 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
| |