dri-devel.lists.freedesktop.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/2] drm/v3d: wait for all jobs to finish before unregistering
@ 2023-10-23 10:58 Maíra Canal
  2023-10-23 10:58 ` [PATCH 2/2] drm/v3d: assure that the job is NULL after being freed Maíra Canal
  2023-10-24  5:57 ` [PATCH 1/2] drm/v3d: wait for all jobs to finish before unregistering Iago Toral
  0 siblings, 2 replies; 6+ messages in thread
From: Maíra Canal @ 2023-10-23 10:58 UTC (permalink / raw)
  To: Emma Anholt, Melissa Wen, Iago Toral, David Airlie,
	Daniel Vetter, Maarten Lankhorst, Maxime Ripard,
	Thomas Zimmermann
  Cc: Maíra Canal, dri-devel

Currently, we are only warning the user if the BIN or RENDER jobs don't
finish before we unregister V3D. We must wait for all jobs to finish
before unregistering. Therefore, warn the user if TFU or CSD jobs
are not done by the time the driver is unregistered.

Signed-off-by: Maíra Canal <mcanal@igalia.com>
---
 drivers/gpu/drm/v3d/v3d_gem.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/v3d/v3d_gem.c b/drivers/gpu/drm/v3d/v3d_gem.c
index 2e94ce788c71..afa7d170d1ff 100644
--- a/drivers/gpu/drm/v3d/v3d_gem.c
+++ b/drivers/gpu/drm/v3d/v3d_gem.c
@@ -1072,6 +1072,8 @@ v3d_gem_destroy(struct drm_device *dev)
 	 */
 	WARN_ON(v3d->bin_job);
 	WARN_ON(v3d->render_job);
+	WARN_ON(v3d->tfu_job);
+	WARN_ON(v3d->csd_job);
 
 	drm_mm_takedown(&v3d->mm);
 
-- 
2.41.0


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH 2/2] drm/v3d: assure that the job is NULL after being freed
  2023-10-23 10:58 [PATCH 1/2] drm/v3d: wait for all jobs to finish before unregistering Maíra Canal
@ 2023-10-23 10:58 ` Maíra Canal
  2023-10-24  5:57 ` [PATCH 1/2] drm/v3d: wait for all jobs to finish before unregistering Iago Toral
  1 sibling, 0 replies; 6+ messages in thread
From: Maíra Canal @ 2023-10-23 10:58 UTC (permalink / raw)
  To: Emma Anholt, Melissa Wen, Iago Toral, David Airlie,
	Daniel Vetter, Maarten Lankhorst, Maxime Ripard,
	Thomas Zimmermann
  Cc: Maíra Canal, dri-devel

After the job is finished and freed, we need to make sure that the job is
NULL. Otherwise, we would get a warning when unloading the driver, as it
would look like the job still exists. Therefore, after freeing the job,
let's assign it to NULL in order to indicate that the job has finished.

Signed-off-by: Maíra Canal <mcanal@igalia.com>
---
 drivers/gpu/drm/v3d/v3d_gem.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/v3d/v3d_gem.c b/drivers/gpu/drm/v3d/v3d_gem.c
index afa7d170d1ff..61a7f36fc8e2 100644
--- a/drivers/gpu/drm/v3d/v3d_gem.c
+++ b/drivers/gpu/drm/v3d/v3d_gem.c
@@ -333,6 +333,7 @@ v3d_job_free(struct kref *ref)
 		v3d_perfmon_put(job->perfmon);

 	kfree(job);
+	job = NULL;
 }

 static void
--
2.41.0


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH 1/2] drm/v3d: wait for all jobs to finish before unregistering
  2023-10-23 10:58 [PATCH 1/2] drm/v3d: wait for all jobs to finish before unregistering Maíra Canal
  2023-10-23 10:58 ` [PATCH 2/2] drm/v3d: assure that the job is NULL after being freed Maíra Canal
@ 2023-10-24  5:57 ` Iago Toral
  2023-10-24 10:05   ` Maira Canal
  1 sibling, 1 reply; 6+ messages in thread
From: Iago Toral @ 2023-10-24  5:57 UTC (permalink / raw)
  To: Maíra Canal, Emma Anholt, Melissa Wen, David Airlie,
	Daniel Vetter, Maarten Lankhorst, Maxime Ripard,
	Thomas Zimmermann
  Cc: dri-devel

El lun, 23-10-2023 a las 07:58 -0300, Maíra Canal escribió:
> Currently, we are only warning the user if the BIN or RENDER jobs
> don't
> finish before we unregister V3D. We must wait for all jobs to finish
> before unregistering. Therefore, warn the user if TFU or CSD jobs
> are not done by the time the driver is unregistered.
> 
> Signed-off-by: Maíra Canal <mcanal@igalia.com>
> ---
>  drivers/gpu/drm/v3d/v3d_gem.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/gpu/drm/v3d/v3d_gem.c
> b/drivers/gpu/drm/v3d/v3d_gem.c
> index 2e94ce788c71..afa7d170d1ff 100644
> --- a/drivers/gpu/drm/v3d/v3d_gem.c
> +++ b/drivers/gpu/drm/v3d/v3d_gem.c
> @@ -1072,6 +1072,8 @@ v3d_gem_destroy(struct drm_device *dev)
>          */
>         WARN_ON(v3d->bin_job);
>         WARN_ON(v3d->render_job);
> +       WARN_ON(v3d->tfu_job);
> +       WARN_ON(v3d->csd_job);

I guess we should do this for cache clean jobs too, right?

Iago

>  
>         drm_mm_takedown(&v3d->mm);
>  


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 1/2] drm/v3d: wait for all jobs to finish before unregistering
  2023-10-24  5:57 ` [PATCH 1/2] drm/v3d: wait for all jobs to finish before unregistering Iago Toral
@ 2023-10-24 10:05   ` Maira Canal
  2023-10-30 12:20     ` Iago Toral
  0 siblings, 1 reply; 6+ messages in thread
From: Maira Canal @ 2023-10-24 10:05 UTC (permalink / raw)
  To: Iago Toral, Emma Anholt, Melissa Wen, David Airlie,
	Daniel Vetter, Maarten Lankhorst, Maxime Ripard,
	Thomas Zimmermann
  Cc: dri-devel

Hi Iago,

On 10/24/23 02:57, Iago Toral wrote:
> El lun, 23-10-2023 a las 07:58 -0300, Maíra Canal escribió:
>> Currently, we are only warning the user if the BIN or RENDER jobs
>> don't
>> finish before we unregister V3D. We must wait for all jobs to finish
>> before unregistering. Therefore, warn the user if TFU or CSD jobs
>> are not done by the time the driver is unregistered.
>>
>> Signed-off-by: Maíra Canal <mcanal@igalia.com>
>> ---
>>   drivers/gpu/drm/v3d/v3d_gem.c | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/v3d/v3d_gem.c
>> b/drivers/gpu/drm/v3d/v3d_gem.c
>> index 2e94ce788c71..afa7d170d1ff 100644
>> --- a/drivers/gpu/drm/v3d/v3d_gem.c
>> +++ b/drivers/gpu/drm/v3d/v3d_gem.c
>> @@ -1072,6 +1072,8 @@ v3d_gem_destroy(struct drm_device *dev)
>>           */
>>          WARN_ON(v3d->bin_job);
>>          WARN_ON(v3d->render_job);
>> +       WARN_ON(v3d->tfu_job);
>> +       WARN_ON(v3d->csd_job);
> 
> I guess we should do this for cache clean jobs too, right?

As the cache clean jobs are synchronous, we don't keep track of the
current cache clean job. When I say that the cache clean jobs are
synchronous, it means that the end of the job is not determined by
an interruption. Therefore, there is no need to make sure that the
cache clean jobs are still running.

Best Regards,
- Maíra

> 
> Iago
> 
>>   
>>          drm_mm_takedown(&v3d->mm);
>>   
> 

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 1/2] drm/v3d: wait for all jobs to finish before unregistering
  2023-10-24 10:05   ` Maira Canal
@ 2023-10-30 12:20     ` Iago Toral
  2023-10-30 12:42       ` Maira Canal
  0 siblings, 1 reply; 6+ messages in thread
From: Iago Toral @ 2023-10-30 12:20 UTC (permalink / raw)
  To: Maira Canal, Emma Anholt, Melissa Wen, David Airlie,
	Daniel Vetter, Maarten Lankhorst, Maxime Ripard,
	Thomas Zimmermann
  Cc: dri-devel

El mar, 24-10-2023 a las 07:05 -0300, Maira Canal escribió:
> Hi Iago,
> 
> On 10/24/23 02:57, Iago Toral wrote:
> > El lun, 23-10-2023 a las 07:58 -0300, Maíra Canal escribió:
> > > Currently, we are only warning the user if the BIN or RENDER jobs
> > > don't
> > > finish before we unregister V3D. We must wait for all jobs to
> > > finish
> > > before unregistering. Therefore, warn the user if TFU or CSD jobs
> > > are not done by the time the driver is unregistered.
> > > 
> > > Signed-off-by: Maíra Canal <mcanal@igalia.com>
> > > ---
> > >   drivers/gpu/drm/v3d/v3d_gem.c | 2 ++
> > >   1 file changed, 2 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/v3d/v3d_gem.c
> > > b/drivers/gpu/drm/v3d/v3d_gem.c
> > > index 2e94ce788c71..afa7d170d1ff 100644
> > > --- a/drivers/gpu/drm/v3d/v3d_gem.c
> > > +++ b/drivers/gpu/drm/v3d/v3d_gem.c
> > > @@ -1072,6 +1072,8 @@ v3d_gem_destroy(struct drm_device *dev)
> > >           */
> > >          WARN_ON(v3d->bin_job);
> > >          WARN_ON(v3d->render_job);
> > > +       WARN_ON(v3d->tfu_job);
> > > +       WARN_ON(v3d->csd_job);
> > 
> > I guess we should do this for cache clean jobs too, right?
> 
> As the cache clean jobs are synchronous, we don't keep track of the
> current cache clean job. When I say that the cache clean jobs are
> synchronous, it means that the end of the job is not determined by
> an interruption. Therefore, there is no need to make sure that the
> cache clean jobs are still running.

I see, thanks Maíra.

Reviewed-by: Iago Toral Quiroga <itoral@igalia.com>

> 
> Best Regards,
> - Maíra
> 
> > 
> > Iago
> > 
> > >   
> > >          drm_mm_takedown(&v3d->mm);
> > >   
> > 
> 


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 1/2] drm/v3d: wait for all jobs to finish before unregistering
  2023-10-30 12:20     ` Iago Toral
@ 2023-10-30 12:42       ` Maira Canal
  0 siblings, 0 replies; 6+ messages in thread
From: Maira Canal @ 2023-10-30 12:42 UTC (permalink / raw)
  To: Iago Toral, Emma Anholt, Melissa Wen, David Airlie,
	Daniel Vetter, Maarten Lankhorst, Maxime Ripard,
	Thomas Zimmermann
  Cc: dri-devel

Hi Iago,

On 10/30/23 09:20, Iago Toral wrote:
> El mar, 24-10-2023 a las 07:05 -0300, Maira Canal escribió:
>> Hi Iago,
>>
>> On 10/24/23 02:57, Iago Toral wrote:
>>> El lun, 23-10-2023 a las 07:58 -0300, Maíra Canal escribió:
>>>> Currently, we are only warning the user if the BIN or RENDER jobs
>>>> don't
>>>> finish before we unregister V3D. We must wait for all jobs to
>>>> finish
>>>> before unregistering. Therefore, warn the user if TFU or CSD jobs
>>>> are not done by the time the driver is unregistered.
>>>>
>>>> Signed-off-by: Maíra Canal <mcanal@igalia.com>
>>>> ---
>>>>    drivers/gpu/drm/v3d/v3d_gem.c | 2 ++
>>>>    1 file changed, 2 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/v3d/v3d_gem.c
>>>> b/drivers/gpu/drm/v3d/v3d_gem.c
>>>> index 2e94ce788c71..afa7d170d1ff 100644
>>>> --- a/drivers/gpu/drm/v3d/v3d_gem.c
>>>> +++ b/drivers/gpu/drm/v3d/v3d_gem.c
>>>> @@ -1072,6 +1072,8 @@ v3d_gem_destroy(struct drm_device *dev)
>>>>            */
>>>>           WARN_ON(v3d->bin_job);
>>>>           WARN_ON(v3d->render_job);
>>>> +       WARN_ON(v3d->tfu_job);
>>>> +       WARN_ON(v3d->csd_job);
>>>
>>> I guess we should do this for cache clean jobs too, right?
>>
>> As the cache clean jobs are synchronous, we don't keep track of the
>> current cache clean job. When I say that the cache clean jobs are
>> synchronous, it means that the end of the job is not determined by
>> an interruption. Therefore, there is no need to make sure that the
>> cache clean jobs are still running.
> 
> I see, thanks Maíra.
> 
> Reviewed-by: Iago Toral Quiroga <itoral@igalia.com>

Applied to drm-misc/drm-misc-next!

Thanks,
- Maíra

> 
>>
>> Best Regards,
>> - Maíra
>>
>>>
>>> Iago
>>>
>>>>    
>>>>           drm_mm_takedown(&v3d->mm);
>>>>    
>>>
>>
> 

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2023-10-30 12:42 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-10-23 10:58 [PATCH 1/2] drm/v3d: wait for all jobs to finish before unregistering Maíra Canal
2023-10-23 10:58 ` [PATCH 2/2] drm/v3d: assure that the job is NULL after being freed Maíra Canal
2023-10-24  5:57 ` [PATCH 1/2] drm/v3d: wait for all jobs to finish before unregistering Iago Toral
2023-10-24 10:05   ` Maira Canal
2023-10-30 12:20     ` Iago Toral
2023-10-30 12:42       ` Maira Canal

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).