Re: [PATCH 02/16] drm/i915: fix the FBC work allocation failure path

From: Daniel Vetter <daniel@ffwll.ch>
To: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
Cc: Intel Graphics Development <intel-gfx@lists.freedesktop.org>
Subject: Re: [PATCH 02/16] drm/i915: fix the FBC work allocation failure path
Date: Wed, 2 Sep 2015 09:52:02 +0200	[thread overview]
Message-ID: <20150902075202.GU1367@phenom.ffwll.local> (raw)
In-Reply-To: <20150901110334.GD29811@intel.com>

On Tue, Sep 01, 2015 at 02:03:34PM +0300, Ville Syrjälä wrote:
> On Tue, Sep 01, 2015 at 12:07:01PM +0200, Daniel Vetter wrote:
> > On Fri, Aug 28, 2015 at 11:50:08AM -0300, Paulo Zanoni wrote:
> > > 2015-08-28 11:20 GMT-03:00 Ville Syrjälä <ville.syrjala@linux.intel.com>:
> > > > On Fri, Aug 14, 2015 at 06:34:07PM -0300, Paulo Zanoni wrote:
> > > >> Always update the currrent crtc, fb and vertical offset after calling
> > > >> enable_fbc. We were forgetting to do so along the failure paths when
> > > >> enabling fbc synchronously. Fix this with a new helper to enable_fbc()
> > > >> and update the state simultaneously.
> > > >>
> > > >> v2: Improve commit message (Chris).
> > > >>
> > > >> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > >> ---
> > > >>  drivers/gpu/drm/i915/intel_fbc.c | 27 +++++++++++++++++----------
> > > >>  1 file changed, 17 insertions(+), 10 deletions(-)
> > > >>
> > > >> diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
> > > >> index c97aba2..fa9b004 100644
> > > >> --- a/drivers/gpu/drm/i915/intel_fbc.c
> > > >> +++ b/drivers/gpu/drm/i915/intel_fbc.c
> > > >> @@ -308,6 +308,18 @@ bool intel_fbc_enabled(struct drm_i915_private *dev_priv)
> > > >>       return dev_priv->fbc.enabled;
> > > >>  }
> > > >>
> > > >> +static void intel_fbc_enable(struct intel_crtc *crtc,
> > > >> +                          struct drm_framebuffer *fb)
> > > >
> > > > fb could be const
> > > 
> > > I guess a lot of things could be constified, if we decide to do this.
> > 
> > Personally I like const on .data (especially vfunc tables since those are
> > nice to create exploits if they're writable and you can get at them). And
> > for core functions/vfuncs where the const has a semantic meaning.
> > Otherwise I personally don't see to much value in sprinkling const all
> > over.
> 
> I especially like making display mode, state, etc. structs const to make
> it clear which functions can change them and which can't. IMO
> drm_framebuffer could use the same treatment.

Yeah I guess making a few things in the drm core static could be useful
indeed, for example plane_state->fb reall should be static, or
crtc_state->mode_blob (or any other fb or blob pointer in state
structures). Same for all the {plane, connector, crtc}->state pointers
(although that would need some casts in swap_states()). I'm not too sure
how much benefit there is if we do this just in i915, since if the
top-level entry points called from drm core aren't enforcing constness
trying to do it in i915 only will probably be an endless effort of fixing
things up all the time.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx