All of lore.kernel.org
 help / color / mirror / Atom feed
From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
Cc: dri-devel@lists.freedesktop.org
Subject: Re: [PATCH 19/32] drm: Store a pointer to drm_format_info under drm_framebuffer
Date: Thu, 17 Nov 2016 20:16:46 +0200	[thread overview]
Message-ID: <30195047.2EPGAeHLHX@avalon> (raw)
In-Reply-To: <20161117181342.GB31595@intel.com>

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@linux.intel.com wrote:
> >>> From: Ville Syrjälä <ville.syrjala@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@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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

  reply	other threads:[~2016-11-17 18:16 UTC|newest]

Thread overview: 52+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-11-17 16:13 [PATCH 00/32] drm: Deduplicate fb format information ville.syrjala
2016-11-17 16:14 ` [PATCH 01/32] drm/i915: Add local 'fb' variables ville.syrjala
2016-11-17 16:14 ` [PATCH 02/32] drm/radeon: " ville.syrjala
2016-11-17 17:21   ` Alex Deucher
2016-11-17 16:14 ` [PATCH 03/32] drm/radeon: Use DIV_ROUND_UP() ville.syrjala
2016-11-17 17:22   ` Alex Deucher
2016-11-17 16:14 ` [PATCH 04/32] drm/mgag200: Add local 'fb' variable ville.syrjala
2016-11-17 16:14 ` [PATCH 05/32] drm/ast: Add local 'fb' variables ville.syrjala
2016-11-17 16:14 ` [PATCH 06/32] drm/gma500: Add some " ville.syrjala
2016-11-17 16:14 ` [PATCH 07/32] drm/cirrus: " ville.syrjala
2016-11-17 16:14 ` [PATCH 08/32] drm/arcpgu: Add " ville.syrjala
2016-11-17 16:14 ` [PATCH 09/32] drm/arm: " ville.syrjala
2016-11-17 16:14 ` [PATCH 10/32] drm/nouveau: Fix crtc->primary->fb vs. drm_fb fail ville.syrjala
2016-11-17 16:14 ` [PATCH 11/32] drm/nouveau: Add local 'fb' variables ville.syrjala
2016-11-17 16:14 ` [PATCH 12/32] drm/vmwgfx: Populate fb->dev before drm_framebuffer_init() ville.syrjala
2016-11-17 16:14 ` [PATCH 13/32] drm: Pass 'dev' to drm_helper_mode_fill_fb_struct() ville.syrjala
2016-11-17 17:41   ` Laurent Pinchart
2016-11-17 16:14 ` [PATCH 14/32] drm/vmwgfx: Populate fb->pixel_format ville.syrjala
2016-11-17 17:52   ` Laurent Pinchart
2016-11-17 19:27     ` Ville Syrjälä
2016-11-17 16:14 ` [PATCH 15/32] drm/qxl: Call drm_helper_mode_fill_fb_struct() before drm_framebuffer_init() ville.syrjala
2016-11-17 16:14 ` [PATCH 16/32] drm/virtio: " ville.syrjala
2016-11-17 16:14 ` [PATCH 17/32] drm/i915: Set fb->dev early on for inherited fbs ville.syrjala
2016-11-17 16:14 ` [PATCH 18/32] drm: Populate fb->dev from drm_helper_mode_fill_fb_struct() ville.syrjala
2016-11-17 17:43   ` Laurent Pinchart
2016-11-17 16:14 ` [PATCH 19/32] drm: Store a pointer to drm_format_info under drm_framebuffer ville.syrjala
2016-11-17 17:57   ` Laurent Pinchart
2016-11-17 18:06     ` Ville Syrjälä
2016-11-17 18:13       ` Ville Syrjälä
2016-11-17 18:16         ` Laurent Pinchart [this message]
2016-11-17 16:14 ` [PATCH 20/32] drm/vmwgfx: Populate fb->format correctly ville.syrjala
2016-11-17 16:14 ` [PATCH 21/32] drm/i915: Populate fb->format early for inherited fbs ville.syrjala
2016-11-17 16:14 ` [PATCH 22/32] drm/atomic: Replace drm_format_num_planes() with fb->format->num_planes ville.syrjala
2016-11-17 17:58   ` Laurent Pinchart
2016-11-17 16:14 ` [PATCH 23/32] drm/fb_cma_helper: Replace drm_format_info() with fb->format ville.syrjala
2016-11-17 17:58   ` Laurent Pinchart
2016-11-17 16:14 ` [PATCH 24/32] drm/nouveau: Use fb->format rather than drm_format_info() ville.syrjala
2016-11-17 16:14 ` [PATCH 25/32] drm/i915: Store a pointer to the pixel format info for fbc ville.syrjala
2016-11-17 16:14 ` [PATCH 26/32] drm/i915: Replace drm_format_plane_cpp() with fb->format->cpp[] ville.syrjala
2016-11-17 16:14 ` [PATCH 27/32] drm/i915: Replace drm_format_num_planes() with fb->format->num_planes ville.syrjala
2016-11-17 16:14 ` [PATCH 28/32] drm: Add drm_framebuffer_plane_{width,height}() ville.syrjala
2016-11-17 16:14 ` [PATCH 29/32] drm/i915: Use drm_framebuffer_plane_{width, height}() where possible ville.syrjala
2016-11-17 16:14 ` [PATCH 30/32] drm: Nuke fb->depth ville.syrjala
2016-11-17 18:03   ` Laurent Pinchart
2016-11-17 16:14 ` [PATCH 31/32] drm: Nuke fb->bits_per_pixel ville.syrjala
2016-11-17 18:14   ` Laurent Pinchart
2016-11-17 18:35     ` Ville Syrjälä
2016-11-17 18:41       ` Laurent Pinchart
2016-11-17 19:18   ` [PATCH v2 " ville.syrjala
2016-11-17 16:14 ` [PATCH 32/32] drm: Nuke fb->pixel_format ville.syrjala
2016-11-17 17:37   ` Alex Deucher
2016-11-17 18:39   ` Laurent Pinchart

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=30195047.2EPGAeHLHX@avalon \
    --to=laurent.pinchart@ideasonboard.com \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=ville.syrjala@linux.intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.