tjh.dev / kernel
Linux kernel mirror (for testing) git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
kernel os linux
1
fork

Configure Feed

Select the types of activity you want to include in your feed.

drm/v3d: Use raw seqcount helpers instead of fighting with lockdep

The `v3d_stats` sequence counter uses regular seqcount helpers, which
carry lockdep annotations that expect a consistent IRQ context between
all writers. However, lockdep is unable to detect that v3d's readers
are never in IRQ or softirq context, and that for CPU job queues, even
the write side never is. This led to false positive that were previously
worked around by conditionally disabling local IRQs under
IS_ENABLED(CONFIG_LOCKDEP).

Switch to the raw seqcount helpers which skip lockdep tracking entirely.
This is safe because jobs are fully serialized per queue: the next job
can only be queued after the previous one has been signaled, so there is
no scope for the start and update paths to race on the same seqcount.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Reviewed-by: Iago Toral Quiroga <itoral@igalia.com>
Link: https://patch.msgid.link/20260306-v3d-reset-locking-improv-v3-2-49864fe00692@igalia.com
Co-developed-by: Maíra Canal <mcanal@igalia.com>
Signed-off-by: Maíra Canal <mcanal@igalia.com>

authored by

Tvrtko Ursulin and committed by
Maíra Canal 0b2a4569 8cf1bec3

+16 -45
+1 -1
drivers/gpu/drm/v3d/v3d_drv.c
··· 194 194 unsigned int seq; 195 195 196 196 do { 197 - seq = read_seqcount_begin(&stats->lock); 197 + seq = raw_read_seqcount_begin(&stats->lock); 198 198 *active_runtime = stats->enabled_ns; 199 199 if (stats->start_ns) 200 200 *active_runtime += timestamp - stats->start_ns;
+5
drivers/gpu/drm/v3d/v3d_drv.h
··· 46 46 * This seqcount is used to protect the access to the GPU stats 47 47 * variables. It must be used as, while we are reading the stats, 48 48 * IRQs can happen and the stats can be updated. 49 + * 50 + * However, we use the raw seqcount helpers to interact with this lock 51 + * to avoid false positives from lockdep, which is unable to detect that 52 + * our readers are never from irq or softirq context, and that, for CPU 53 + * job queues, even the write side never is. 49 54 */ 50 55 seqcount_t lock; 51 56 };
+10 -44
drivers/gpu/drm/v3d/v3d_sched.c
··· 144 144 struct v3d_stats *global_stats = &v3d->queue[queue].stats; 145 145 struct v3d_stats *local_stats = &file->stats[queue]; 146 146 u64 now = local_clock(); 147 - unsigned long flags; 148 147 149 - /* 150 - * We only need to disable local interrupts to appease lockdep who 151 - * otherwise would think v3d_job_start_stats vs v3d_stats_update has an 152 - * unsafe in-irq vs no-irq-off usage problem. This is a false positive 153 - * because all the locks are per queue and stats type, and all jobs are 154 - * completely one at a time serialised. More specifically: 155 - * 156 - * 1. Locks for GPU queues are updated from interrupt handlers under a 157 - * spin lock and started here with preemption disabled. 158 - * 159 - * 2. Locks for CPU queues are updated from the worker with preemption 160 - * disabled and equally started here with preemption disabled. 161 - * 162 - * Therefore both are consistent. 163 - * 164 - * 3. Because next job can only be queued after the previous one has 165 - * been signaled, and locks are per queue, there is also no scope for 166 - * the start part to race with the update part. 167 - */ 168 - if (IS_ENABLED(CONFIG_LOCKDEP)) 169 - local_irq_save(flags); 170 - else 171 - preempt_disable(); 148 + preempt_disable(); 172 149 173 - write_seqcount_begin(&local_stats->lock); 150 + raw_write_seqcount_begin(&local_stats->lock); 174 151 local_stats->start_ns = now; 175 - write_seqcount_end(&local_stats->lock); 152 + raw_write_seqcount_end(&local_stats->lock); 176 153 177 - write_seqcount_begin(&global_stats->lock); 154 + raw_write_seqcount_begin(&global_stats->lock); 178 155 global_stats->start_ns = now; 179 - write_seqcount_end(&global_stats->lock); 156 + raw_write_seqcount_end(&global_stats->lock); 180 157 181 - if (IS_ENABLED(CONFIG_LOCKDEP)) 182 - local_irq_restore(flags); 183 - else 184 - preempt_enable(); 158 + preempt_enable(); 185 159 } 186 160 187 161 static void 188 162 v3d_stats_update(struct v3d_stats *stats, u64 now) 189 163 { 190 - write_seqcount_begin(&stats->lock); 164 + raw_write_seqcount_begin(&stats->lock); 191 165 stats->enabled_ns += now - stats->start_ns; 192 166 stats->jobs_completed++; 193 167 stats->start_ns = 0; 194 - write_seqcount_end(&stats->lock); 168 + raw_write_seqcount_end(&stats->lock); 195 169 } 196 170 197 171 void ··· 175 201 struct v3d_queue_state *queue = &v3d->queue[q]; 176 202 struct v3d_stats *global_stats = &queue->stats; 177 203 u64 now = local_clock(); 178 - unsigned long flags; 179 204 180 - /* See comment in v3d_job_start_stats() */ 181 - if (IS_ENABLED(CONFIG_LOCKDEP)) 182 - local_irq_save(flags); 183 - else 184 - preempt_disable(); 205 + preempt_disable(); 185 206 186 207 /* Don't update the local stats if the file context has already closed */ 187 208 spin_lock(&queue->queue_lock); ··· 186 217 187 218 v3d_stats_update(global_stats, now); 188 219 189 - if (IS_ENABLED(CONFIG_LOCKDEP)) 190 - local_irq_restore(flags); 191 - else 192 - preempt_enable(); 220 + preempt_enable(); 193 221 } 194 222 195 223 static struct dma_fence *v3d_bin_job_run(struct drm_sched_job *sched_job)