[PATCH 1/3] drm/etnaviv: move debug register en-/disable into own function

[Lucas Stach]

Am Freitag, dem 28.06.2024 um 13:07 +0200 schrieb Philipp Zabel:
> On Fr, 2024-06-28 at 12:47 +0200, Lucas Stach wrote:
> > The next changes will introduce another place where the debug registers
> > need to be en-/disabled. Split into separate functions, so we don't need
> > to replicate the code there. Also allow those calls to nest, keeping
> > the debug registers enabled until all callers don't need them any longer.
> > 
> > Signed-off-by: Lucas Stach <l.stach at pengutronix.de>
> > ---
> >  drivers/gpu/drm/etnaviv/etnaviv_gpu.c | 33 ++++++++++++++++++++-------
> >  drivers/gpu/drm/etnaviv/etnaviv_gpu.h |  3 +++
> >  2 files changed, 28 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
> > index 7c7f97793ddd..ade6f7554706 100644
> > --- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
> > +++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
> > @@ -471,6 +471,29 @@ static void etnaviv_hw_identify(struct etnaviv_gpu *gpu)
> >  
> >  	etnaviv_hw_specs(gpu);
> >  }
> > +void etnaviv_gpu_enable_debug_regs(struct etnaviv_gpu *gpu)
> > +{
> > +	u32 val;
> > +
> > +	if (atomic_inc_return(&gpu->dbg_ref) > 1)
> > +		return;
> 
> This is a reference count, any reason not to use refcount_t?

refcount_t doesn't provide the nice _return interface and I don't think
the memory related messages emitted by refcount on misuse are helpful
here.

> 
> > +
> > +	val = gpu_read(gpu, VIVS_HI_CLOCK_CONTROL);
> > +	val &= ~VIVS_HI_CLOCK_CONTROL_DISABLE_DEBUG_REGISTERS;
> > +	gpu_write(gpu, VIVS_HI_CLOCK_CONTROL, val);
> 
> Does this need locking after patch 3, to avoid racing of
> sync_point_perfmon_sample_pre/post() with etnaviv_sched_timedout_job()?
> 
Right, the enable path is racy and may cause one of the racing threads
to read the debug registers before they have been enabled. This will
need proper locking.

Regards,
Lucas