Re: [PATCH 4/6] drm/i915: Allow mmio updates on all platforms.

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Thu, Mar 24, 2016 at 09:35:04AM +0100, Maarten Lankhorst wrote:
> Op 23-03-16 om 16:07 schreef Ville Syrjälä:
> > On Wed, Mar 23, 2016 at 02:24:30PM +0100, Maarten Lankhorst wrote:
> >> Rename intel_unpin_work to intel_flip_work and use it for mmio flips
> >> and unpinning. Use flip_queued_req to hold the wait request in the
> >> mmio case and allow the vblank interrupt to complete mmio work to
> >> have mmio flips run correctly on g4 and earlier.
> > Before you actually go and trust drm_vblank_count() you should make it
> > race free.
> 
> How about adding the below to the patch?

You can't just mix the hw and sw counter. Using the hw counter would be
neat because it doesn't require so much work to be race-free, but that
leaves out gen2 which I don't like. My atomic code used the hw counter
though, but I had plans to fall back to the sw counter on gen2
eventually.

> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index befac649aa96..05ec832e02de 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11224,12 +11224,11 @@ static void intel_mmio_flip_work_func(struct work_struct *w)
>  							    false, false,
>  							    MAX_SCHEDULE_TIMEOUT) < 0);
>  
> -	intel_pipe_update_start(crtc);
> +	intel_pipe_update_start(crtc, work);
>  	primary->update_plane(&primary->base,
>  			      crtc->config,
>  			      to_intel_plane_state(primary->base.state));
> -	atomic_set(&work->pending, INTEL_FLIP_PENDING);
> -	intel_pipe_update_end(crtc);
> +	intel_pipe_update_end(crtc, work);
>  }
>  
>  static int intel_default_queue_flip(struct drm_device *dev,
> @@ -13800,7 +13799,7 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc,
>  	bool modeset = needs_modeset(crtc->state);
>  
>  	/* Perform vblank evasion around commit operation */
> -	intel_pipe_update_start(intel_crtc);
> +	intel_pipe_update_start(intel_crtc, NULL);
>  
>  	if (modeset)
>  		return;
> @@ -13816,7 +13815,7 @@ static void intel_finish_crtc_commit(struct drm_crtc *crtc,
>  {
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  
> -	intel_pipe_update_end(intel_crtc);
> +	intel_pipe_update_end(intel_crtc, NULL);
>  }
>  
>  /**
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index cac368138764..86d486cfd778 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1604,8 +1604,8 @@ bool intel_sdvo_init(struct drm_device *dev,
>  int intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane);
>  int intel_sprite_set_colorkey(struct drm_device *dev, void *data,
>  			      struct drm_file *file_priv);
> -void intel_pipe_update_start(struct intel_crtc *crtc);
> -void intel_pipe_update_end(struct intel_crtc *crtc);
> +void intel_pipe_update_start(struct intel_crtc *crtc, struct intel_flip_work *work);
> +void intel_pipe_update_end(struct intel_crtc *crtc, struct intel_flip_work *work);
>  
>  /* intel_tv.c */
>  void intel_tv_init(struct drm_device *dev);
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index 8821533561b1..8da59a1eca56 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -78,7 +78,8 @@ static int usecs_to_scanlines(const struct drm_display_mode *adjusted_mode,
>   * avoid random delays. The value written to @start_vbl_count should be
>   * supplied to intel_pipe_update_end() for error checking.
>   */
> -void intel_pipe_update_start(struct intel_crtc *crtc)
> +void intel_pipe_update_start(struct intel_crtc *crtc,
> +			     struct intel_flip_work *work)
>  {
>  	struct drm_device *dev = crtc->base.dev;
>  	const struct drm_display_mode *adjusted_mode = &crtc->config->base.adjusted_mode;
> @@ -142,6 +143,9 @@ void intel_pipe_update_start(struct intel_crtc *crtc)
>  	crtc->debug.start_vbl_count =
>  		dev->driver->get_vblank_counter(dev, pipe);
>  
> +	if (work)
> +		work->flip_queued_vblank = crtc->debug.start_vbl_count;
> +
>  	trace_i915_pipe_update_vblank_evaded(crtc);
>  }
>  
> @@ -154,7 +158,7 @@ void intel_pipe_update_start(struct intel_crtc *crtc)
>   * re-enables interrupts and verifies the update was actually completed
>   * before a vblank using the value of @start_vbl_count.
>   */
> -void intel_pipe_update_end(struct intel_crtc *crtc)
> +void intel_pipe_update_end(struct intel_crtc *crtc, struct intel_flip_work *work)
>  {
>  	struct drm_device *dev = crtc->base.dev;
>  	enum pipe pipe = crtc->pipe;
> @@ -162,6 +166,12 @@ void intel_pipe_update_end(struct intel_crtc *crtc)
>  	u32 end_vbl_count = dev->driver->get_vblank_counter(dev, pipe);
>  	ktime_t end_vbl_time = ktime_get();
>  
> +	if (work) {
> +		work->flip_queued_vblank = end_vbl_count;
> +		smp_mb__before_atomic();
> +		atomic_set(&work->pending, INTEL_FLIP_PENDING);
> +	}
> +
>  	trace_i915_pipe_update_end(crtc, end_vbl_count, scanline_end);
>  
>  	local_irq_enable();

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx