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@ffwll.ch
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@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel