[PATCH 19/32] drm: Store a pointer to drm_format_info under drm_framebuffer

[Laurent Pinchart]

Hi Ville,

On Thursday 17 Nov 2016 20:13:42 Ville Syrjälä wrote:
> On Thu, Nov 17, 2016 at 08:06:10PM +0200, Ville Syrjälä wrote:
> > On Thu, Nov 17, 2016 at 07:57:27PM +0200, Laurent Pinchart wrote:
> >> On Thursday 17 Nov 2016 18:14:18 ville.syrjala at linux.intel.com wrote:
> >>> From: Ville Syrjälä <ville.syrjala at linux.intel.com>
> >>> 
> >>> To avoid having to look up the format information struct every time,
> >>> let's just store a pointer to it under drm_framebuffer.
> >>> 
> >>> Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> >>> ---
> >>> 
> >>>  drivers/gpu/drm/drm_framebuffer.c    | 4 +++-
> >>>  drivers/gpu/drm/drm_modeset_helper.c | 1 +
> >>>  include/drm/drm_framebuffer.h        | 4 ++++
> >>>  3 files changed, 8 insertions(+), 1 deletion(-)
> >>> 
> >>> diff --git a/drivers/gpu/drm/drm_framebuffer.c
> >>> b/drivers/gpu/drm/drm_framebuffer.c index 527220c08f9b..47478678d609
> >>> 100644
> >>> --- a/drivers/gpu/drm/drm_framebuffer.c
> >>> +++ b/drivers/gpu/drm/drm_framebuffer.c
> >>> @@ -632,8 +632,10 @@ int drm_framebuffer_init(struct drm_device *dev,
> >>> struct drm_framebuffer *fb, int ret;
> >>> 
> >>>  	INIT_LIST_HEAD(&fb->filp_head);
> >>> 
> >>> -	if (WARN_ON_ONCE(fb->dev != dev))
> >>> +	if (WARN_ON_ONCE(fb->dev != dev)) {
> >>>  		fb->dev = dev;
> >>> +		fb->format = drm_format_info(fb->pixel_format);
> >>> +	}
> >> 
> >> With this drivers will start relying on fb->format. I believe we should
> >> commit to provide a non-NULL format info to avoid having to sprinkle
> >> NULL checks in all drivers. Should drm_framebuffer_init() return an
> >> error in that case ? Or do we guarantee already through another mean
> >> that the pixel format is valid and will always lead to a non-NULL
> >> format info here ?
> > 
> > I think for normal cases we already reject the NULL format info. I am a
> > little worried that I might have missed some special case where that
> > might not hold. But I'm not sure what would be the best way to guard
> > against it. I really don't want to sprinkle NULL checks everywhere, and
> > in fact I didn't.
> 
> And by normal I mean the addfb/addfb2 ioctls. Those will go through
> framebuffer_check() which does:
> 
> info = __drm_format_info(...)
> if (!info) {
> 	...
> 	return -EINVAL;
> }
> 
> Well, you should know since you write the code ;)
> 
> So the more special cases of internally created fbs is where the danger
> lies I think.
> 
> I guess we could have drm_framebuffer_init() return a failure (also
> maybe in the fb->dev!=dev case?). That would at least protect all the
> uses after drm_framebuffer_init(). And I guess the uses before _init()
> are somewhat of a special case, and up to each driver to make sure that
> it knows what it's doing if it has uses like that.

That sounds good to me.

> >>>  	fb->funcs = funcs;
> >>>  	
> >>>  	ret = drm_mode_object_get_reg(dev, &fb->base, DRM_MODE_OBJECT_FB,

[snip]

-- 
Regards,

Laurent Pinchart