On Thu, Jul 28, 2016 at 12:02:01PM +0200, Daniel Vetter wrote:
> On Wed, Jul 27, 2016 at 12:14:54PM +0100, Chris Wilson wrote:
> > We only need a very lightweight mechanism here as the locking is only
> > used for co-ordinating a bitfield.
> >
> > v2: Move the cheap unlikely tests into the caller
> >
> > Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
>
> I think the code shuffling in here badly breaks the kerneldoc. Best fix
> would be to extract a small header for frontbuffer tracking and pull that
> into the kernel doc. Much less preferred is to explicitly pull in the
> kerneldoc function-by-function (but that tends to be rather fragile when
> someone adds something new).
fwiw the change itself looks correct and makes sense, so with
intel_frontbuffer.h extracted and the kerneldoc include directives in
gpu/drm-i915.rst fixed up this is:
Reviewed-by: Daniel Vetter <daniel.vetter@xxxxxxxx>
> -Daniel
>
> > ---
> > drivers/gpu/drm/i915/i915_drv.h | 2 +-
> > drivers/gpu/drm/i915/i915_gem.c | 2 +-
> > drivers/gpu/drm/i915/intel_drv.h | 29 ++++++++++++++---
> > drivers/gpu/drm/i915/intel_frontbuffer.c | 54 ++++++++++++++------------------
> > 4 files changed, 51 insertions(+), 36 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 3a68e604ad10..a24d31e3e014 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -1669,7 +1669,7 @@ struct intel_pipe_crc {
> > };
> >
> > struct i915_frontbuffer_tracking {
> > - struct mutex lock;
> > + spinlock_t lock;
> >
> > /*
> > * Tracking bits for delayed frontbuffer flushing du to gpu activity or
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > index 1fb958dcc749..7db0808f6961 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -4447,7 +4447,7 @@ i915_gem_load_init(struct drm_device *dev)
> >
> > dev_priv->mm.interruptible = true;
> >
> > - mutex_init(&dev_priv->fb_tracking.lock);
> > + spin_lock_init(&dev_priv->fb_tracking.lock);
> > }
> >
> > void i915_gem_load_cleanup(struct drm_device *dev)
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index e74d851868c5..01056ce8d461 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -1135,8 +1135,6 @@ void intel_ddi_set_vc_payload_alloc(struct drm_crtc *crtc, bool state);
> > uint32_t ddi_signal_levels(struct intel_dp *intel_dp);
> >
> > /* intel_frontbuffer.c */
> > -void intel_fb_obj_invalidate(struct drm_i915_gem_object *obj,
> > - enum fb_op_origin origin);
> > void intel_frontbuffer_flip_prepare(struct drm_device *dev,
> > unsigned frontbuffer_bits);
> > void intel_frontbuffer_flip_complete(struct drm_device *dev,
> > @@ -1147,8 +1145,31 @@ unsigned int intel_fb_align_height(struct drm_device *dev,
> > unsigned int height,
> > uint32_t pixel_format,
> > uint64_t fb_format_modifier);
> > -void intel_fb_obj_flush(struct drm_i915_gem_object *obj, bool retire,
> > - enum fb_op_origin origin);
> > +
> > +void __intel_fb_obj_invalidate(struct drm_i915_gem_object *obj,
> > + enum fb_op_origin origin);
> > +static inline void intel_fb_obj_invalidate(struct drm_i915_gem_object *obj,
> > + enum fb_op_origin origin)
> > +{
> > + if (!obj->frontbuffer_bits)
> > + return;
> > +
> > + __intel_fb_obj_invalidate(obj, origin);
> > +}
> > +
> > +void __intel_fb_obj_flush(struct drm_i915_gem_object *obj,
> > + bool retire,
> > + enum fb_op_origin origin);
> > +static inline void intel_fb_obj_flush(struct drm_i915_gem_object *obj,
> > + bool retire,
> > + enum fb_op_origin origin)
> > +{
> > + if (!obj->frontbuffer_bits)
> > + return;
> > +
> > + __intel_fb_obj_flush(obj, retire, origin);
> > +}
> > +
> > u32 intel_fb_stride_alignment(const struct drm_i915_private *dev_priv,
> > uint64_t fb_modifier, uint32_t pixel_format);
> >
> > diff --git a/drivers/gpu/drm/i915/intel_frontbuffer.c b/drivers/gpu/drm/i915/intel_frontbuffer.c
> > index ac85357010b4..a38ccfe4894a 100644
> > --- a/drivers/gpu/drm/i915/intel_frontbuffer.c
> > +++ b/drivers/gpu/drm/i915/intel_frontbuffer.c
> > @@ -76,24 +76,19 @@
> > * until the rendering completes or a flip on this frontbuffer plane is
> > * scheduled.
> > */
> > -void intel_fb_obj_invalidate(struct drm_i915_gem_object *obj,
> > - enum fb_op_origin origin)
> > +void __intel_fb_obj_invalidate(struct drm_i915_gem_object *obj,
> > + enum fb_op_origin origin)
> > {
> > struct drm_device *dev = obj->base.dev;
> > struct drm_i915_private *dev_priv = to_i915(dev);
> >
> > WARN_ON(!mutex_is_locked(&dev->struct_mutex));
> >
> > - if (!obj->frontbuffer_bits)
> > - return;
> > -
> > if (origin == ORIGIN_CS) {
> > - mutex_lock(&dev_priv->fb_tracking.lock);
> > - dev_priv->fb_tracking.busy_bits
> > - |= obj->frontbuffer_bits;
> > - dev_priv->fb_tracking.flip_bits
> > - &= ~obj->frontbuffer_bits;
> > - mutex_unlock(&dev_priv->fb_tracking.lock);
> > + spin_lock(&dev_priv->fb_tracking.lock);
> > + dev_priv->fb_tracking.busy_bits |= obj->frontbuffer_bits;
> > + dev_priv->fb_tracking.flip_bits &= ~obj->frontbuffer_bits;
> > + spin_unlock(&dev_priv->fb_tracking.lock);
> > }
> >
> > intel_psr_invalidate(dev, obj->frontbuffer_bits);
> > @@ -120,11 +115,11 @@ static void intel_frontbuffer_flush(struct drm_device *dev,
> > struct drm_i915_private *dev_priv = to_i915(dev);
> >
> > /* Delay flushing when rings are still busy.*/
> > - mutex_lock(&dev_priv->fb_tracking.lock);
> > + spin_lock(&dev_priv->fb_tracking.lock);
> > frontbuffer_bits &= ~dev_priv->fb_tracking.busy_bits;
> > - mutex_unlock(&dev_priv->fb_tracking.lock);
> > + spin_unlock(&dev_priv->fb_tracking.lock);
> >
> > - if (!frontbuffer_bits)
> > + if (frontbuffer_bits == 0)
> > return;
> >
> > intel_edp_drrs_flush(dev, frontbuffer_bits);
> > @@ -142,8 +137,9 @@ static void intel_frontbuffer_flush(struct drm_device *dev,
> > * completed and frontbuffer caching can be started again. If @retire is true
> > * then any delayed flushes will be unblocked.
> > */
> > -void intel_fb_obj_flush(struct drm_i915_gem_object *obj,
> > - bool retire, enum fb_op_origin origin)
> > +void __intel_fb_obj_flush(struct drm_i915_gem_object *obj,
> > + bool retire,
> > + enum fb_op_origin origin)
> > {
> > struct drm_device *dev = obj->base.dev;
> > struct drm_i915_private *dev_priv = to_i915(dev);
> > @@ -151,21 +147,18 @@ void intel_fb_obj_flush(struct drm_i915_gem_object *obj,
> >
> > WARN_ON(!mutex_is_locked(&dev->struct_mutex));
> >
> > - if (!obj->frontbuffer_bits)
> > - return;
> > -
> > frontbuffer_bits = obj->frontbuffer_bits;
> >
> > if (retire) {
> > - mutex_lock(&dev_priv->fb_tracking.lock);
> > + spin_lock(&dev_priv->fb_tracking.lock);
> > /* Filter out new bits since rendering started. */
> > frontbuffer_bits &= dev_priv->fb_tracking.busy_bits;
> > -
> > dev_priv->fb_tracking.busy_bits &= ~frontbuffer_bits;
> > - mutex_unlock(&dev_priv->fb_tracking.lock);
> > + spin_unlock(&dev_priv->fb_tracking.lock);
> > }
> >
> > - intel_frontbuffer_flush(dev, frontbuffer_bits, origin);
> > + if (frontbuffer_bits)
> > + intel_frontbuffer_flush(dev, frontbuffer_bits, origin);
> > }
> >
> > /**
> > @@ -185,11 +178,11 @@ void intel_frontbuffer_flip_prepare(struct drm_device *dev,
> > {
> > struct drm_i915_private *dev_priv = to_i915(dev);
> >
> > - mutex_lock(&dev_priv->fb_tracking.lock);
> > + spin_lock(&dev_priv->fb_tracking.lock);
> > dev_priv->fb_tracking.flip_bits |= frontbuffer_bits;
> > /* Remove stale busy bits due to the old buffer. */
> > dev_priv->fb_tracking.busy_bits &= ~frontbuffer_bits;
> > - mutex_unlock(&dev_priv->fb_tracking.lock);
> > + spin_unlock(&dev_priv->fb_tracking.lock);
> >
> > intel_psr_single_frame_update(dev, frontbuffer_bits);
> > }
> > @@ -209,13 +202,14 @@ void intel_frontbuffer_flip_complete(struct drm_device *dev,
> > {
> > struct drm_i915_private *dev_priv = to_i915(dev);
> >
> > - mutex_lock(&dev_priv->fb_tracking.lock);
> > + spin_lock(&dev_priv->fb_tracking.lock);
> > /* Mask any cancelled flips. */
> > frontbuffer_bits &= dev_priv->fb_tracking.flip_bits;
> > dev_priv->fb_tracking.flip_bits &= ~frontbuffer_bits;
> > - mutex_unlock(&dev_priv->fb_tracking.lock);
> > + spin_unlock(&dev_priv->fb_tracking.lock);
> >
> > - intel_frontbuffer_flush(dev, frontbuffer_bits, ORIGIN_FLIP);
> > + if (frontbuffer_bits)
> > + intel_frontbuffer_flush(dev, frontbuffer_bits, ORIGIN_FLIP);
> > }
> >
> > /**
> > @@ -234,10 +228,10 @@ void intel_frontbuffer_flip(struct drm_device *dev,
> > {
> > struct drm_i915_private *dev_priv = to_i915(dev);
> >
> > - mutex_lock(&dev_priv->fb_tracking.lock);
> > + spin_lock(&dev_priv->fb_tracking.lock);
> > /* Remove stale busy bits due to the old buffer. */
> > dev_priv->fb_tracking.busy_bits &= ~frontbuffer_bits;
> > - mutex_unlock(&dev_priv->fb_tracking.lock);
> > + spin_unlock(&dev_priv->fb_tracking.lock);
> >
> > intel_frontbuffer_flush(dev, frontbuffer_bits, ORIGIN_FLIP);
> > }
> > --
> > 2.8.1
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
