Re: [Intel-gfx] [PATCH] drm: Handle unexpected holes in color-eviction
Quoting Daniel Vetter (2018-02-19 15:52:34)
> On Mon, Feb 19, 2018 at 02:43:59PM +, 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(>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, >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
Not too surprising that no one else had an opinion since only i915 uses
the node coloring at present. Pushed to drm-misc-fixes.
> I spent some brain cycles trying to come up with clever tricks to avoid
> the need to this function outright, but failed.
I think it's not too bad overall. When scanning we include the necessary
bookends in the hole calculation, so we know that if we do need to evict
a bookend then it is available and was included in our LRU ordering. It
is just that during the scan, the intermediate state is inconsistent
with the final arrangement and so instead of always evicting those
bookends we make a final call on whether the new node can fit into the
smaller hole or needs the enlargement. It should only be called a maximum
of two times to check on the two bookends, and have no impact for when
coloring is not in effect.
-Chris
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm: Handle unexpected holes in color-eviction
On Mon, Feb 19, 2018 at 02:43:59PM +, 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(>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, >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
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_stack == >hole_stack);
> > > + if (unlikely(>hole_stack == >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-de...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm: Handle unexpected holes in color-eviction
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(>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, >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_stack == >hole_stack);
> > + if (unlikely(>hole_stack == >hole_stack))
>
> Would be more readable as:
>
> if (...) {
> DRM_MM_BUG()
> }
Maybe DRM_MM_WARN_ON(), both hypothetical functions :)
-Chris
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm: Handle unexpected holes in color-eviction
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(>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, >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?
> + break;
> + }
> +
> + /* We should only be called after we found the hole previously */
> + DRM_MM_BUG_ON(>hole_stack == >hole_stack);
> + if (unlikely(>hole_stack == >hole_stack))
Would be more readable as:
if (...) {
DRM_MM_BUG()
}
Reviewed-by: Joonas Lahtinen
Regards, Joonas
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
