Quoting Joonas Lahtinen (2018-02-19 14:40:32)
> Quoting Chris Wilson (2018-02-19 13:35:43)
> > +++ b/drivers/gpu/drm/drm_mm.c
> > @@ -836,9 +836,24 @@ struct drm_mm_node *drm_mm_scan_color_evict(struct drm_mm_scan *scan)
> > if (!mm->color_adjust)
> > return NULL;
> >
> > - hole = list_first_entry(&mm->hole_stack, typeof(*hole), hole_stack);
> > - hole_start = __drm_mm_hole_node_start(hole);
> > - hole_end = hole_start + hole->hole_size;
> > + /*
> > + * The hole found during scanning should ideally be the first element
> > + * in the hole_stack list, but due to side-effects in the driver it
> > + * may not be.
> > + */
> > + list_for_each_entry(hole, &mm->hole_stack, hole_stack) {
> > + hole_start = __drm_mm_hole_node_start(hole);
> > + hole_end = hole_start + hole->hole_size;
> > +
> > + if (hole_start <= scan->hit_start &&
> > + hole_end >= scan->hit_end)
>
> How about some likely() here?
I felt that at the point where we admit we need a search, likely() went
out of the window. Given that I think we'll need 2 or 3 iterations at
most, that probably means in practice this is around the 50% mark.
Claiming that it's likely() may be misleading to the reader.
> > + break;
> > + }
> > +
> > + /* We should only be called after we found the hole previously */
> > + DRM_MM_BUG_ON(&hole->hole_stack == &mm->hole_stack);
> > + if (unlikely(&hole->hole_stack == &mm->hole_stack))
>
> Would be more readable as:
>
> if (...) {
> DRM_MM_BUG()
> }
Maybe DRM_MM_WARN_ON(), both hypothetical functions :)
-Chris