On Mon, Feb 19, 2018 at 02:43:59PM +0000, Chris Wilson wrote: > 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. Concurred. Reviewed-by: Daniel Vetter <daniel.vetter@xxxxxxxx> I spent some brain cycles trying to come up with clever tricks to avoid the need to this function outright, but failed. -Daniel > > > + 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 > _______________________________________________ > dri-devel mailing list > dri-devel@xxxxxxxxxxxxxxxxxxxxx > https://lists.freedesktop.org/mailman/listinfo/dri-devel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch