Re: [PATCH] drm: Handle unexpected holes in color-eviction

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [Linux Kernel]     [Kernel Development Newbies]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Hiking]     [Linux Kernel]     [Linux SCSI]