[PATCH 2/6] drm/omap: gem: Merge __omap_gem_get_pages() and omap_gem_attach_pages()

[Laurent Pinchart]

Hello,

On Wednesday, 4 April 2018 19:18:42 EEST Daniel Vetter wrote:
> On Wed, Apr 4, 2018 at 3:37 PM, Lucas Stach <l.stach at pengutronix.de> wrote:
> > Am Montag, den 02.04.2018, 21:50 +0300 schrieb Laurent Pinchart:
> >> The __omap_gem_get_pages() function is a wrapper around
> >> omap_gem_attach_pages() that returns the omap_obj->pages pointer through
> >> a function argument. Some callers don't need the pages pointer, and all
> >> of them can access omap_obj->pages directly. To simplify the code merge
> >> the __omap_gem_get_pages() wrapper with omap_gem_attach_pages() and
> >> update the callers accordingly.
> >> 
> >> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> >> 
> >> ---
> >> 
> >>  drivers/gpu/drm/omapdrm/omap_gem.c | 62 ++++++++++++--------------------
> >>  1 file changed, 23 insertions(+), 39 deletions(-)
> >> 
> >> diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c
> >> b/drivers/gpu/drm/omapdrm/omap_gem.c index 6cfcf60cffe3..13fea207343e
> >> 100644
> >> --- a/drivers/gpu/drm/omapdrm/omap_gem.c
> >> +++ b/drivers/gpu/drm/omapdrm/omap_gem.c
> >> @@ -222,7 +222,10 @@ static void omap_gem_evict(struct drm_gem_object
> >> *obj)
> >>   * Page Management
> >>   */
> >> 
> >> -/** ensure backing pages are allocated */
> >> +/*
> >> + * Ensure backing pages are allocated. Must be called with the
> >> omap_obj.lock
> >> + * held.
> >> + */
> > 
> > Drive-by comment: I always find it hard to validate those comment-only
> > lock prerequisites, especially if callstacks get deeper.
> > 
> > What we do in etnaviv is to make those lock comments executable by
> > using lockdep_assert_held() to validate the locking assumptions. This
> > makes sure that if you ever manage to violate the locking in a code
> > rework, a lockdep enabled debug build will explode right at the spot.
> 
> +1 on this. I've gone as far as removing all the locking related
> comments in core drm code because most of it was misleading or
> outright wrong. The runtime checks have a much higher chance of
> actually being correct :-)

I agree, I'll fix that in the next version. I plan to keep the comment though, 
as I find it easier to read when glancing at the function, but I'll add a 
corresponding lockdep_assert_held().

[snip]

-- 
Regards,

Laurent Pinchart