Re: [PATCH 04/25] drm/i915/fbc: introduce struct intel_fbc_reg_params

["Zanoni, Paulo R" <paulo.r.zanoni@intel.com>]

Em Qui, 2016-01-21 às 13:48 +0100, Maarten Lankhorst escreveu:
> Op 19-01-16 om 14:35 schreef Paulo Zanoni:
> > The early return inside __intel_fbc_update does not completely
> > check
> > all the parameters that affect the FBC register values. For
> > example,
> > we currently lack looking at crtc->adjusted_y (for the fence Y
> > offset)
> > and all the parameters that affect the CFB size (for i8xx).
> > 
> > Instead of just adding the missing parameters to the check and
> > hoping
> > that any changes to the fbc_activate functions also come with a
> > matching change to the __intel_fbc_update check, introduce a new
> > structure where we store these parameters and use the structure at
> > the
> > fbc_activate function. Of course, it's still possible to access
> > everything from dev_priv in those functions, but IMHO the new code
> > will be harder to break.
> > 
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h  |  22 ++++++-
> >  drivers/gpu/drm/i915/intel_fbc.c | 131 ++++++++++++++++++++++-----
> > ------------
> >  2 files changed, 93 insertions(+), 60 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > b/drivers/gpu/drm/i915/i915_drv.h
> > index 33217a4..aa9c35e 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -909,11 +909,9 @@ struct i915_fbc {
> >  	 * it's the outer lock when overlapping with stolen_lock.
> > */
> >  	struct mutex lock;
> >  	unsigned threshold;
> > -	unsigned int fb_id;
> >  	unsigned int possible_framebuffer_bits;
> >  	unsigned int busy_bits;
> >  	struct intel_crtc *crtc;
> > -	int y;
> >  
> >  	struct drm_mm_node compressed_fb;
> >  	struct drm_mm_node *compressed_llb;
> > @@ -923,6 +921,24 @@ struct i915_fbc {
> >  	bool enabled;
> >  	bool active;
> >  
> > +	struct intel_fbc_reg_params {
> > +		struct {
> > +			enum pipe pipe;
> > +			enum plane plane;
> > +			unsigned int fence_y_offset;
> > +		} crtc;
> > +
> > +		struct {
> > +			u64 ggtt_offset;
> > +			uint32_t id;
> On a casual glance it looks like this fb.id is write-only now, but
> the whole struct is being compared.

It is read during intel_fbc_reg_params_equal(), so we're still
comparing it. I tried to be conservative and just remove the fb.id
comparison later, just to make bisecting easier in case something
happens. But we remove fb.id in patch 22/25.

> 
> It might be worth making a note about that.
> ~Maarten
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx