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@xxxxxxxxxxxxxxx wrote:
> >>> From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> >>>
> >>> 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@xxxxxxxxxxxxxxx>
> >>> ---
> >>>
> >>> 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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
