Message ID | 20231028155936.1183342-4-stanislaw.gruszka@linux.intel.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | accel/ivpu: Update for -next 2023-10-28 | expand |
On 10/28/2023 9:59 AM, Stanislaw Gruszka wrote: > Stop job_done thread when going to suspend. Use kthread_park() instead > of kthread_stop() to avoid memory allocation and potential failure > on resume. > > Use separate function as thread wake up condition. Use spin lock to assure > rx_msg_list is properly protected against concurrent access. This tells me what you are doing (and the changes seems to make sense), but doesn't say "why". A quick, one-sentence justification would fix this I think. > Reviewed-by: Karol Wachowski <karol.wachowski@linux.intel.com> > Signed-off-by: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com> Reviewed-by: Jeffrey Hugo <quic_jhugo@quicinc.com>
This conflicts with the kthread change in 6.7-rc1 6309727ef271 ("kthread: add kthread_stop_put") Please double-check that the conflict resolution I've done in drm-tip is correct and then ask drm-misc/accel maintainers to backmerge -rc2 to bake this in properly. Adding them as fyi. -Sima On Sat, 28 Oct 2023 at 18:00, Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com> wrote: > > Stop job_done thread when going to suspend. Use kthread_park() instead > of kthread_stop() to avoid memory allocation and potential failure > on resume. > > Use separate function as thread wake up condition. Use spin lock to assure > rx_msg_list is properly protected against concurrent access. > > Reviewed-by: Karol Wachowski <karol.wachowski@linux.intel.com> > Signed-off-by: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com> > --- > drivers/accel/ivpu/ivpu_drv.c | 2 ++ > drivers/accel/ivpu/ivpu_ipc.c | 17 +++++++++++++++-- > drivers/accel/ivpu/ivpu_job.c | 20 ++++++++++++++++---- > drivers/accel/ivpu/ivpu_job.h | 2 ++ > 4 files changed, 35 insertions(+), 6 deletions(-) > > diff --git a/drivers/accel/ivpu/ivpu_drv.c b/drivers/accel/ivpu/ivpu_drv.c > index 064cabef41bb..60277ff6af69 100644 > --- a/drivers/accel/ivpu/ivpu_drv.c > +++ b/drivers/accel/ivpu/ivpu_drv.c > @@ -378,6 +378,7 @@ int ivpu_boot(struct ivpu_device *vdev) > enable_irq(vdev->irq); > ivpu_hw_irq_enable(vdev); > ivpu_ipc_enable(vdev); > + ivpu_job_done_thread_enable(vdev); > return 0; > } > > @@ -389,6 +390,7 @@ int ivpu_shutdown(struct ivpu_device *vdev) > disable_irq(vdev->irq); > ivpu_ipc_disable(vdev); > ivpu_mmu_disable(vdev); > + ivpu_job_done_thread_disable(vdev); > > ret = ivpu_hw_power_down(vdev); > if (ret) > diff --git a/drivers/accel/ivpu/ivpu_ipc.c b/drivers/accel/ivpu/ivpu_ipc.c > index d069d1e1f91d..270caef789bf 100644 > --- a/drivers/accel/ivpu/ivpu_ipc.c > +++ b/drivers/accel/ivpu/ivpu_ipc.c > @@ -202,6 +202,20 @@ ivpu_ipc_send(struct ivpu_device *vdev, struct ivpu_ipc_consumer *cons, struct v > return ret; > } > > +static int ivpu_ipc_rx_need_wakeup(struct ivpu_ipc_consumer *cons) > +{ > + int ret = 0; > + > + if (IS_KTHREAD()) > + ret |= (kthread_should_stop() || kthread_should_park()); > + > + spin_lock_irq(&cons->rx_msg_lock); > + ret |= !list_empty(&cons->rx_msg_list); > + spin_unlock_irq(&cons->rx_msg_lock); > + > + return ret; > +} > + > int ivpu_ipc_receive(struct ivpu_device *vdev, struct ivpu_ipc_consumer *cons, > struct ivpu_ipc_hdr *ipc_buf, > struct vpu_jsm_msg *ipc_payload, unsigned long timeout_ms) > @@ -211,8 +225,7 @@ int ivpu_ipc_receive(struct ivpu_device *vdev, struct ivpu_ipc_consumer *cons, > int wait_ret, ret = 0; > > wait_ret = wait_event_interruptible_timeout(cons->rx_msg_wq, > - (IS_KTHREAD() && kthread_should_stop()) || > - !list_empty(&cons->rx_msg_list), > + ivpu_ipc_rx_need_wakeup(cons), > msecs_to_jiffies(timeout_ms)); > > if (IS_KTHREAD() && kthread_should_stop()) > diff --git a/drivers/accel/ivpu/ivpu_job.c b/drivers/accel/ivpu/ivpu_job.c > index 6e96c921547d..a245b2d44db7 100644 > --- a/drivers/accel/ivpu/ivpu_job.c > +++ b/drivers/accel/ivpu/ivpu_job.c > @@ -590,6 +590,11 @@ static int ivpu_job_done_thread(void *arg) > ivpu_pm_schedule_recovery(vdev); > } > } > + if (kthread_should_park()) { > + ivpu_dbg(vdev, JOB, "Parked %s\n", __func__); > + kthread_parkme(); > + ivpu_dbg(vdev, JOB, "Unparked %s\n", __func__); > + } > } > > ivpu_ipc_consumer_del(vdev, &cons); > @@ -610,9 +615,6 @@ int ivpu_job_done_thread_init(struct ivpu_device *vdev) > return -EIO; > } > > - get_task_struct(thread); > - wake_up_process(thread); > - > vdev->job_done_thread = thread; > > return 0; > @@ -620,6 +622,16 @@ int ivpu_job_done_thread_init(struct ivpu_device *vdev) > > void ivpu_job_done_thread_fini(struct ivpu_device *vdev) > { > + kthread_unpark(vdev->job_done_thread); > kthread_stop(vdev->job_done_thread); > - put_task_struct(vdev->job_done_thread); > +} > + > +void ivpu_job_done_thread_disable(struct ivpu_device *vdev) > +{ > + kthread_park(vdev->job_done_thread); > +} > + > +void ivpu_job_done_thread_enable(struct ivpu_device *vdev) > +{ > + kthread_unpark(vdev->job_done_thread); > } > diff --git a/drivers/accel/ivpu/ivpu_job.h b/drivers/accel/ivpu/ivpu_job.h > index aa1f0b9479b0..a8e914e5affc 100644 > --- a/drivers/accel/ivpu/ivpu_job.h > +++ b/drivers/accel/ivpu/ivpu_job.h > @@ -61,6 +61,8 @@ void ivpu_cmdq_reset_all_contexts(struct ivpu_device *vdev); > > int ivpu_job_done_thread_init(struct ivpu_device *vdev); > void ivpu_job_done_thread_fini(struct ivpu_device *vdev); > +void ivpu_job_done_thread_disable(struct ivpu_device *vdev); > +void ivpu_job_done_thread_enable(struct ivpu_device *vdev); > > void ivpu_jobs_abort_all(struct ivpu_device *vdev); > > -- > 2.25.1 >
Hi On Mon, Nov 13, 2023 at 08:39:42AM +0100, Daniel Vetter wrote: > This conflicts with the kthread change in 6.7-rc1 6309727ef271 > ("kthread: add kthread_stop_put") > > Please double-check that the conflict resolution I've done in drm-tip > is correct and then ask drm-misc/accel maintainers to backmerge -rc2 > to bake this in properly. Adding them as fyi. Can not rebuild drm-tip due to core-to-CI merge problem. However resolution in drm-rerere commit fa53f5a2888265d883eedb83a943613a410b9fc9 is correct. Thomas please do the backmarge. Regards Stanislaw > On Sat, 28 Oct 2023 at 18:00, Stanislaw Gruszka > <stanislaw.gruszka@linux.intel.com> wrote: > > > > Stop job_done thread when going to suspend. Use kthread_park() instead > > of kthread_stop() to avoid memory allocation and potential failure > > on resume. > > > > Use separate function as thread wake up condition. Use spin lock to assure > > rx_msg_list is properly protected against concurrent access. > > > > Reviewed-by: Karol Wachowski <karol.wachowski@linux.intel.com> > > Signed-off-by: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com> > > --- > > drivers/accel/ivpu/ivpu_drv.c | 2 ++ > > drivers/accel/ivpu/ivpu_ipc.c | 17 +++++++++++++++-- > > drivers/accel/ivpu/ivpu_job.c | 20 ++++++++++++++++---- > > drivers/accel/ivpu/ivpu_job.h | 2 ++ > > 4 files changed, 35 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/accel/ivpu/ivpu_drv.c b/drivers/accel/ivpu/ivpu_drv.c > > index 064cabef41bb..60277ff6af69 100644 > > --- a/drivers/accel/ivpu/ivpu_drv.c > > +++ b/drivers/accel/ivpu/ivpu_drv.c > > @@ -378,6 +378,7 @@ int ivpu_boot(struct ivpu_device *vdev) > > enable_irq(vdev->irq); > > ivpu_hw_irq_enable(vdev); > > ivpu_ipc_enable(vdev); > > + ivpu_job_done_thread_enable(vdev); > > return 0; > > } > > > > @@ -389,6 +390,7 @@ int ivpu_shutdown(struct ivpu_device *vdev) > > disable_irq(vdev->irq); > > ivpu_ipc_disable(vdev); > > ivpu_mmu_disable(vdev); > > + ivpu_job_done_thread_disable(vdev); > > > > ret = ivpu_hw_power_down(vdev); > > if (ret) > > diff --git a/drivers/accel/ivpu/ivpu_ipc.c b/drivers/accel/ivpu/ivpu_ipc.c > > index d069d1e1f91d..270caef789bf 100644 > > --- a/drivers/accel/ivpu/ivpu_ipc.c > > +++ b/drivers/accel/ivpu/ivpu_ipc.c > > @@ -202,6 +202,20 @@ ivpu_ipc_send(struct ivpu_device *vdev, struct ivpu_ipc_consumer *cons, struct v > > return ret; > > } > > > > +static int ivpu_ipc_rx_need_wakeup(struct ivpu_ipc_consumer *cons) > > +{ > > + int ret = 0; > > + > > + if (IS_KTHREAD()) > > + ret |= (kthread_should_stop() || kthread_should_park()); > > + > > + spin_lock_irq(&cons->rx_msg_lock); > > + ret |= !list_empty(&cons->rx_msg_list); > > + spin_unlock_irq(&cons->rx_msg_lock); > > + > > + return ret; > > +} > > + > > int ivpu_ipc_receive(struct ivpu_device *vdev, struct ivpu_ipc_consumer *cons, > > struct ivpu_ipc_hdr *ipc_buf, > > struct vpu_jsm_msg *ipc_payload, unsigned long timeout_ms) > > @@ -211,8 +225,7 @@ int ivpu_ipc_receive(struct ivpu_device *vdev, struct ivpu_ipc_consumer *cons, > > int wait_ret, ret = 0; > > > > wait_ret = wait_event_interruptible_timeout(cons->rx_msg_wq, > > - (IS_KTHREAD() && kthread_should_stop()) || > > - !list_empty(&cons->rx_msg_list), > > + ivpu_ipc_rx_need_wakeup(cons), > > msecs_to_jiffies(timeout_ms)); > > > > if (IS_KTHREAD() && kthread_should_stop()) > > diff --git a/drivers/accel/ivpu/ivpu_job.c b/drivers/accel/ivpu/ivpu_job.c > > index 6e96c921547d..a245b2d44db7 100644 > > --- a/drivers/accel/ivpu/ivpu_job.c > > +++ b/drivers/accel/ivpu/ivpu_job.c > > @@ -590,6 +590,11 @@ static int ivpu_job_done_thread(void *arg) > > ivpu_pm_schedule_recovery(vdev); > > } > > } > > + if (kthread_should_park()) { > > + ivpu_dbg(vdev, JOB, "Parked %s\n", __func__); > > + kthread_parkme(); > > + ivpu_dbg(vdev, JOB, "Unparked %s\n", __func__); > > + } > > } > > > > ivpu_ipc_consumer_del(vdev, &cons); > > @@ -610,9 +615,6 @@ int ivpu_job_done_thread_init(struct ivpu_device *vdev) > > return -EIO; > > } > > > > - get_task_struct(thread); > > - wake_up_process(thread); > > - > > vdev->job_done_thread = thread; > > > > return 0; > > @@ -620,6 +622,16 @@ int ivpu_job_done_thread_init(struct ivpu_device *vdev) > > > > void ivpu_job_done_thread_fini(struct ivpu_device *vdev) > > { > > + kthread_unpark(vdev->job_done_thread); > > kthread_stop(vdev->job_done_thread); > > - put_task_struct(vdev->job_done_thread); > > +} > > + > > +void ivpu_job_done_thread_disable(struct ivpu_device *vdev) > > +{ > > + kthread_park(vdev->job_done_thread); > > +} > > + > > +void ivpu_job_done_thread_enable(struct ivpu_device *vdev) > > +{ > > + kthread_unpark(vdev->job_done_thread); > > } > > diff --git a/drivers/accel/ivpu/ivpu_job.h b/drivers/accel/ivpu/ivpu_job.h > > index aa1f0b9479b0..a8e914e5affc 100644 > > --- a/drivers/accel/ivpu/ivpu_job.h > > +++ b/drivers/accel/ivpu/ivpu_job.h > > @@ -61,6 +61,8 @@ void ivpu_cmdq_reset_all_contexts(struct ivpu_device *vdev); > > > > int ivpu_job_done_thread_init(struct ivpu_device *vdev); > > void ivpu_job_done_thread_fini(struct ivpu_device *vdev); > > +void ivpu_job_done_thread_disable(struct ivpu_device *vdev); > > +void ivpu_job_done_thread_enable(struct ivpu_device *vdev); > > > > void ivpu_jobs_abort_all(struct ivpu_device *vdev); > > > > -- > > 2.25.1 > > > > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch
diff --git a/drivers/accel/ivpu/ivpu_drv.c b/drivers/accel/ivpu/ivpu_drv.c index 064cabef41bb..60277ff6af69 100644 --- a/drivers/accel/ivpu/ivpu_drv.c +++ b/drivers/accel/ivpu/ivpu_drv.c @@ -378,6 +378,7 @@ int ivpu_boot(struct ivpu_device *vdev) enable_irq(vdev->irq); ivpu_hw_irq_enable(vdev); ivpu_ipc_enable(vdev); + ivpu_job_done_thread_enable(vdev); return 0; } @@ -389,6 +390,7 @@ int ivpu_shutdown(struct ivpu_device *vdev) disable_irq(vdev->irq); ivpu_ipc_disable(vdev); ivpu_mmu_disable(vdev); + ivpu_job_done_thread_disable(vdev); ret = ivpu_hw_power_down(vdev); if (ret) diff --git a/drivers/accel/ivpu/ivpu_ipc.c b/drivers/accel/ivpu/ivpu_ipc.c index d069d1e1f91d..270caef789bf 100644 --- a/drivers/accel/ivpu/ivpu_ipc.c +++ b/drivers/accel/ivpu/ivpu_ipc.c @@ -202,6 +202,20 @@ ivpu_ipc_send(struct ivpu_device *vdev, struct ivpu_ipc_consumer *cons, struct v return ret; } +static int ivpu_ipc_rx_need_wakeup(struct ivpu_ipc_consumer *cons) +{ + int ret = 0; + + if (IS_KTHREAD()) + ret |= (kthread_should_stop() || kthread_should_park()); + + spin_lock_irq(&cons->rx_msg_lock); + ret |= !list_empty(&cons->rx_msg_list); + spin_unlock_irq(&cons->rx_msg_lock); + + return ret; +} + int ivpu_ipc_receive(struct ivpu_device *vdev, struct ivpu_ipc_consumer *cons, struct ivpu_ipc_hdr *ipc_buf, struct vpu_jsm_msg *ipc_payload, unsigned long timeout_ms) @@ -211,8 +225,7 @@ int ivpu_ipc_receive(struct ivpu_device *vdev, struct ivpu_ipc_consumer *cons, int wait_ret, ret = 0; wait_ret = wait_event_interruptible_timeout(cons->rx_msg_wq, - (IS_KTHREAD() && kthread_should_stop()) || - !list_empty(&cons->rx_msg_list), + ivpu_ipc_rx_need_wakeup(cons), msecs_to_jiffies(timeout_ms)); if (IS_KTHREAD() && kthread_should_stop()) diff --git a/drivers/accel/ivpu/ivpu_job.c b/drivers/accel/ivpu/ivpu_job.c index 6e96c921547d..a245b2d44db7 100644 --- a/drivers/accel/ivpu/ivpu_job.c +++ b/drivers/accel/ivpu/ivpu_job.c @@ -590,6 +590,11 @@ static int ivpu_job_done_thread(void *arg) ivpu_pm_schedule_recovery(vdev); } } + if (kthread_should_park()) { + ivpu_dbg(vdev, JOB, "Parked %s\n", __func__); + kthread_parkme(); + ivpu_dbg(vdev, JOB, "Unparked %s\n", __func__); + } } ivpu_ipc_consumer_del(vdev, &cons); @@ -610,9 +615,6 @@ int ivpu_job_done_thread_init(struct ivpu_device *vdev) return -EIO; } - get_task_struct(thread); - wake_up_process(thread); - vdev->job_done_thread = thread; return 0; @@ -620,6 +622,16 @@ int ivpu_job_done_thread_init(struct ivpu_device *vdev) void ivpu_job_done_thread_fini(struct ivpu_device *vdev) { + kthread_unpark(vdev->job_done_thread); kthread_stop(vdev->job_done_thread); - put_task_struct(vdev->job_done_thread); +} + +void ivpu_job_done_thread_disable(struct ivpu_device *vdev) +{ + kthread_park(vdev->job_done_thread); +} + +void ivpu_job_done_thread_enable(struct ivpu_device *vdev) +{ + kthread_unpark(vdev->job_done_thread); } diff --git a/drivers/accel/ivpu/ivpu_job.h b/drivers/accel/ivpu/ivpu_job.h index aa1f0b9479b0..a8e914e5affc 100644 --- a/drivers/accel/ivpu/ivpu_job.h +++ b/drivers/accel/ivpu/ivpu_job.h @@ -61,6 +61,8 @@ void ivpu_cmdq_reset_all_contexts(struct ivpu_device *vdev); int ivpu_job_done_thread_init(struct ivpu_device *vdev); void ivpu_job_done_thread_fini(struct ivpu_device *vdev); +void ivpu_job_done_thread_disable(struct ivpu_device *vdev); +void ivpu_job_done_thread_enable(struct ivpu_device *vdev); void ivpu_jobs_abort_all(struct ivpu_device *vdev);