Re: [PATCH v8 3/9] drm/i915: Make sprite updates atomic

Re: [PATCH v8 3/9] drm/i915: Make sprite updates atomic

[Arun Murthy]

[-- Attachment #1.1: Type: text/plain, Size: 1459 bytes --]

 Add a mechanism by which we can evade the leading edge of vblank. This
guarantees that no two sprite register writes will straddle on either
side of the vblank start, and that means all the writes will be latched
together in one atomic operation.

Here only one sprite update followed by the primary enable/disable can be
achieved atomically. But I feel update of all planes are to be considered,
i.e
update of planes per pipe basis to achieve atomicity.


We do the vblank evade by checking the scanline counter, and if it's too
close to the start of vblank (too close has been hardcoded to 100usec
for now), we will wait for the vblank start to pass. In order to
eliminate random delayes from the rest of the system, we operate with
interrupts disabled, except when waiting for the vblank obviously.

This can be achieved easily by checking the previous vblank time in
drm_get_last_vblanktimestamp(), using this the next vblank time cab be
predicted. Using these with the hardcoded value 100usec, a check can be
made to continue or to wait for a vblank. For waiting for a vblank instead
of adding new function just use the available intel_wait_for_vblank().

last_vblank = drm_get_last_vblanktimestamp();
curr_time = do_getttimeofday();
if ((last_vblank +  VBLANK_TIME_INTERVAL) - curr_time.tv_usec > 100)
    /* acquire lock and proceed*/
else
    /* wait for one vblank, acquire lock and proceed */

Thanks and Regards,
Arun R Murthy
-------------------

[-- Attachment #1.2: Type: text/html, Size: 1999 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v8 3/9] drm/i915: Make sprite updates atomic

[Ville Syrjälä]

On Mon, May 19, 2014 at 02:49:59PM +0530, Arun Murthy wrote:
>  Add a mechanism by which we can evade the leading edge of vblank. This
> guarantees that no two sprite register writes will straddle on either
> side of the vblank start, and that means all the writes will be latched
> together in one atomic operation.
> 
> Here only one sprite update followed by the primary enable/disable can be
> achieved atomically. But I feel update of all planes are to be considered,
> i.e
> update of planes per pipe basis to achieve atomicity.
> 
> 
> We do the vblank evade by checking the scanline counter, and if it's too
> close to the start of vblank (too close has been hardcoded to 100usec
> for now), we will wait for the vblank start to pass. In order to
> eliminate random delayes from the rest of the system, we operate with
> interrupts disabled, except when waiting for the vblank obviously.
> 
> This can be achieved easily by checking the previous vblank time in
> drm_get_last_vblanktimestamp(), using this the next vblank time cab be
> predicted. Using these with the hardcoded value 100usec, a check can be
> made to continue or to wait for a vblank. For waiting for a vblank instead
> of adding new function just use the available intel_wait_for_vblank().
> 
> last_vblank = drm_get_last_vblanktimestamp();
> curr_time = do_getttimeofday();
> if ((last_vblank +  VBLANK_TIME_INTERVAL) - curr_time.tv_usec > 100)
>     /* acquire lock and proceed*/
> else
>     /* wait for one vblank, acquire lock and proceed */

Sure, if your aim is to make it less robust.

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH v8 3/9] drm/i915: Make sprite updates atomic

[Ville Syrjälä]

On Thu, May 22, 2014 at 03:03:56PM +0200, Daniel Vetter wrote:
> On Tue, Apr 29, 2014 at 01:35:46PM +0300, ville.syrjala@linux.intel.com wrote:
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index 96ae78d..d8b540b 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -401,6 +401,8 @@ struct intel_crtc {
> >  		/* watermarks currently being used  */
> >  		struct intel_pipe_wm active;
> >  	} wm;
> > +
> > +	wait_queue_head_t vbl_wait;
> >  };
> 
> A bit late, but found something: Why can't we use dev->vblank[crtc].queue
> here? We reuse all the vblank infrastructure already ...
> 
> If you want pretty just add a tiny helper function
> 
> wait_queue_head_t *drm_crtc_vblank_waitqueue(struct drm_crtc *crtc)
> {
> 	return &dev->vblank[drm_crtc_index(crtc)].queue;
> }
> 
> somewhere.
> 
> I feel guilty here since I've killed Akash's patch over adding a new
> vblank wait queue and let yours slip through ;-) Can you please take care
> of this?

Can do.

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH v8 3/9] drm/i915: Make sprite updates atomic

[Daniel Vetter]

On Tue, Apr 29, 2014 at 01:35:46PM +0300, ville.syrjala@linux.intel.com wrote:
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 96ae78d..d8b540b 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -401,6 +401,8 @@ struct intel_crtc {
>  		/* watermarks currently being used  */
>  		struct intel_pipe_wm active;
>  	} wm;
> +
> +	wait_queue_head_t vbl_wait;
>  };

A bit late, but found something: Why can't we use dev->vblank[crtc].queue
here? We reuse all the vblank infrastructure already ...

If you want pretty just add a tiny helper function

wait_queue_head_t *drm_crtc_vblank_waitqueue(struct drm_crtc *crtc)
{
	return &dev->vblank[drm_crtc_index(crtc)].queue;
}

somewhere.

I feel guilty here since I've killed Akash's patch over adding a new
vblank wait queue and let yours slip through ;-) Can you please take care
of this?

Thanks, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH v8 3/9] drm/i915: Make sprite updates atomic

[G, Pallavi]

-----Original Message-----
From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of ville.syrjala@linux.intel.com
Sent: Tuesday, April 29, 2014 4:06 PM
To: intel-gfx@lists.freedesktop.org
Subject: [Intel-gfx] [PATCH v8 3/9] drm/i915: Make sprite updates atomic

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Add a mechanism by which we can evade the leading edge of vblank. This guarantees that no two sprite register writes will straddle on either side of the vblank start, and that means all the writes will be latched together in one atomic operation.

We do the vblank evade by checking the scanline counter, and if it's too close to the start of vblank (too close has been hardcoded to 100usec for now), we will wait for the vblank start to pass. In order to eliminate random delayes from the rest of the system, we operate with interrupts disabled, except when waiting for the vblank obviously.

Note that we now go digging through pipe_to_crtc_mapping[] in the vblank interrupt handler, which is a bit dangerous since we set up interrupts before the crtcs. However in this case since it's the vblank interrupt, we don't actually unmask it until some piece of code requests it.

v2: preempt_check_resched() calls after local_irq_enable() (Jesse)
    Hook up the vblank irq stuff on BDW as well
v3: Pass intel_crtc instead of drm_crtc (Daniel)
    Warn if crtc.mutex isn't locked (Daniel)
    Add an explicit compiler barrier and document the barriers (Daniel)
    Note the irq vs. modeset setup madness in the commit message (Daniel)
v4: Use prepare_to_wait() & co. directly and eliminate vbl_received
v5: Refactor intel_pipe_handle_vblank() vs. drm_handle_vblank() (Chris)
    Check for min/max scanline <= 0 (Chris)
    Don't call intel_pipe_update_end() if start failed totally (Chris)
    Check that the vblank counters match on both sides of the critical
    section (Chris)
v6: Fix atomic update for interlaced modes
v7: Reorder code for better readability (Chris)
v8: Drop preempt_check_resched(). It's not available to modules
    anymore and isn't even needed unless we ourselves cause
    a wakeup needing reschedule while interrupts are off

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
Reviewed-by: Sourab Gupta <sourabgupta@gmail.com>
Reviewed-by: Akash Goel <akash.goels@gmail.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c      |  25 +++++--
 drivers/gpu/drm/i915/intel_display.c |   2 +
 drivers/gpu/drm/i915/intel_drv.h     |   2 +
 drivers/gpu/drm/i915/intel_sprite.c  | 131 +++++++++++++++++++++++++++++++++++
 4 files changed, 154 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c index ed30a5e..7e0d577 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1556,6 +1556,19 @@ static void gen6_rps_irq_handler(struct drm_i915_private *dev_priv, u32 pm_iir)
 	}
 }
 
+static bool intel_pipe_handle_vblank(struct drm_device *dev, enum pipe 
+pipe) {
+	struct intel_crtc *crtc;
+
+	if (!drm_handle_vblank(dev, pipe))
+		return false;
+
+	crtc = to_intel_crtc(intel_get_crtc_for_pipe(dev, pipe));
+	wake_up(&crtc->vbl_wait);
+
+	return true;
+}
+
 static void valleyview_pipestat_irq_handler(struct drm_device *dev, u32 iir)  {
 	struct drm_i915_private *dev_priv = dev->dev_private; @@ -1607,7 +1620,7 @@ static void valleyview_pipestat_irq_handler(struct drm_device *dev, u32 iir)
 
 	for_each_pipe(pipe) {
 		if (pipe_stats[pipe] & PIPE_START_VBLANK_INTERRUPT_STATUS)
-			drm_handle_vblank(dev, pipe);
+			intel_pipe_handle_vblank(dev, pipe);
 
 		if (pipe_stats[pipe] & PLANE_FLIP_DONE_INT_STATUS_VLV) {
 			intel_prepare_page_flip(dev, pipe);
@@ -1850,7 +1863,7 @@ static void ilk_display_irq_handler(struct drm_device *dev, u32 de_iir)
 
 	for_each_pipe(pipe) {
 		if (de_iir & DE_PIPE_VBLANK(pipe))
-			drm_handle_vblank(dev, pipe);
+			intel_pipe_handle_vblank(dev, pipe);
 
 		if (de_iir & DE_PIPE_FIFO_UNDERRUN(pipe))
 			if (intel_set_cpu_fifo_underrun_reporting(dev, pipe, false)) @@ -1900,7 +1913,7 @@ static void ivb_display_irq_handler(struct drm_device *dev, u32 de_iir)
 
 	for_each_pipe(pipe) {
 		if (de_iir & (DE_PIPE_VBLANK_IVB(pipe)))
-			drm_handle_vblank(dev, pipe);
+			intel_pipe_handle_vblank(dev, pipe);
 
 		/* plane/pipes map 1:1 on ilk+ */
 		if (de_iir & DE_PLANE_FLIP_DONE_IVB(pipe)) { @@ -2043,7 +2056,7 @@ static irqreturn_t gen8_irq_handler(int irq, void *arg)
 
 		pipe_iir = I915_READ(GEN8_DE_PIPE_IIR(pipe));
 		if (pipe_iir & GEN8_PIPE_VBLANK)
-			drm_handle_vblank(dev, pipe);
+			intel_pipe_handle_vblank(dev, pipe);
 
 		if (pipe_iir & GEN8_PIPE_PRIMARY_FLIP_DONE) {
 			intel_prepare_page_flip(dev, pipe);
@@ -3391,7 +3404,7 @@ static bool i8xx_handle_vblank(struct drm_device *dev,
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	u16 flip_pending = DISPLAY_PLANE_FLIP_PENDING(plane);
 
-	if (!drm_handle_vblank(dev, pipe))
+	if (!intel_pipe_handle_vblank(dev, pipe))
 		return false;
 
 	if ((iir & flip_pending) == 0)
@@ -3576,7 +3589,7 @@ static bool i915_handle_vblank(struct drm_device *dev,
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	u32 flip_pending = DISPLAY_PLANE_FLIP_PENDING(plane);
 
-	if (!drm_handle_vblank(dev, pipe))
+	if (!intel_pipe_handle_vblank(dev, pipe))
 		return false;
 
 	if ((iir & flip_pending) == 0)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index ca33ef5..993597b 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -10566,6 +10566,8 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
 		intel_crtc->plane = !pipe;
 	}
 
+	init_waitqueue_head(&intel_crtc->vbl_wait);
+
 	BUG_ON(pipe >= ARRAY_SIZE(dev_priv->plane_to_crtc_mapping) ||
 	       dev_priv->plane_to_crtc_mapping[intel_crtc->plane] != NULL);
 	dev_priv->plane_to_crtc_mapping[intel_crtc->plane] = &intel_crtc->base; diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 96ae78d..d8b540b 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -401,6 +401,8 @@ struct intel_crtc {
 		/* watermarks currently being used  */
 		struct intel_pipe_wm active;
 	} wm;
+
+	wait_queue_head_t vbl_wait;
 };
 
 struct intel_plane_wm_parameters {
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 336ae6c..a3ed840 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -37,6 +37,89 @@
 #include <drm/i915_drm.h>
 #include "i915_drv.h"
 
+static int usecs_to_scanlines(const struct drm_display_mode *mode, int 
+usecs) {
+	/* paranoia */
+	if (!mode->crtc_htotal)
+		return 1;
+
+	return DIV_ROUND_UP(usecs * mode->crtc_clock, 1000 * 
+mode->crtc_htotal); }
+
+static bool intel_pipe_update_start(struct intel_crtc *crtc, uint32_t 
+*start_vbl_count) {
+	struct drm_device *dev = crtc->base.dev;
+	const struct drm_display_mode *mode = &crtc->config.adjusted_mode;
+	enum pipe pipe = crtc->pipe;
+	long timeout = msecs_to_jiffies_timeout(1);
+	int scanline, min, max, vblank_start;
+	DEFINE_WAIT(wait);
+
+	WARN_ON(!mutex_is_locked(&crtc->base.mutex));
+
+	vblank_start = mode->crtc_vblank_start;
+	if (mode->flags & DRM_MODE_FLAG_INTERLACE)
+		vblank_start = DIV_ROUND_UP(vblank_start, 2);
+
+	/* FIXME needs to be calibrated sensibly */
+	min = vblank_start - usecs_to_scanlines(mode, 100);
+	max = vblank_start - 1;
+
+	if (min <= 0 || max <= 0)
+		return false;
+
+	if (WARN_ON(drm_vblank_get(dev, pipe)))
+		return false;
+
+	local_irq_disable();
+
+	for (;;) {
+		/*
+		 * prepare_to_wait() has a memory barrier, which guarantees
+		 * other CPUs can see the task state update by the time we
+		 * read the scanline.
+		 */
+		prepare_to_wait(&crtc->vbl_wait, &wait, TASK_UNINTERRUPTIBLE);
+
+		scanline = intel_get_crtc_scanline(crtc);
+		if (scanline < min || scanline > max)
+			break;
+
+		if (timeout <= 0) {
+			DRM_ERROR("Potential atomic update failure on pipe %c\n",
+				  pipe_name(crtc->pipe));
+			break;
+		}
+
+		local_irq_enable();
+
+		timeout = schedule_timeout(timeout);
+
+		local_irq_disable();
+	}
+
+	finish_wait(&crtc->vbl_wait, &wait);
+
+	drm_vblank_put(dev, pipe);
+
+	*start_vbl_count = dev->driver->get_vblank_counter(dev, pipe);
+
+	return true;
+}

The irq enable/disable appear to be a costly operation. Am suspecting whether we will get 60fps performance from the sprite setplane update.
Becoz we had faced similar kind of performance issues in VLV when sprite planes are enabled we got only 30fps of performance due to wait_for_vblank call before the old fb unpin.
In case of VLV we have one primary plane and two sprite planes, assume we have some scenario like all the three planes are enabled in android and all the three flip calls (primary_page_flip and two setPlane) will try get/put the vblank  and in between we have irq enable/disable. I hope all these things will be solved once your atomic framework in place but now we are trying to call the pageflip ioctls sequentially and with some WA able to achieve some level of atomicity in android.

Also one more thing in Android is that the HAL is keep on requesting vblank event continuously and report back the vblank timestamp to the window manager and window manager will try to sync all the fb update with respect to the hw vblank time. If we disable/enable the irq in between this will create big kiosk in that path also.

+static void intel_pipe_update_end(struct intel_crtc *crtc, u32 
+start_vbl_count) {
+	struct drm_device *dev = crtc->base.dev;
+	enum pipe pipe = crtc->pipe;
+	u32 end_vbl_count = dev->driver->get_vblank_counter(dev, pipe);
+
+	local_irq_enable();
+
+	if (start_vbl_count != end_vbl_count)
+		DRM_ERROR("Atomic update failure on pipe %c (start=%u end=%u)\n",
+			  pipe_name(pipe), start_vbl_count, end_vbl_count); }
+
 static void
 vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
 		 struct drm_framebuffer *fb,
@@ -48,11 +131,14 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
 	struct drm_device *dev = dplane->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_plane *intel_plane = to_intel_plane(dplane);
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	int pipe = intel_plane->pipe;
 	int plane = intel_plane->plane;
 	u32 sprctl;
 	unsigned long sprsurf_offset, linear_offset;
 	int pixel_size = drm_format_plane_cpp(fb->pixel_format, 0);
+	u32 start_vbl_count;
+	bool atomic_update;
 
 	sprctl = I915_READ(SPCNTR(pipe, plane));
 
@@ -131,6 +217,8 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
 							fb->pitches[0]);
 	linear_offset -= sprsurf_offset;
 
+	atomic_update = intel_pipe_update_start(intel_crtc, &start_vbl_count);
+
 	I915_WRITE(SPSTRIDE(pipe, plane), fb->pitches[0]);
 	I915_WRITE(SPPOS(pipe, plane), (crtc_y << 16) | crtc_x);
 
@@ -144,6 +232,9 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
 	I915_WRITE(SPSURF(pipe, plane), i915_gem_obj_ggtt_offset(obj) +
 		   sprsurf_offset);
 	POSTING_READ(SPSURF(pipe, plane));
+
+	if (atomic_update)
+		intel_pipe_update_end(intel_crtc, start_vbl_count);
 }
 
 static void
@@ -152,8 +243,13 @@ vlv_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc)
 	struct drm_device *dev = dplane->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_plane *intel_plane = to_intel_plane(dplane);
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	int pipe = intel_plane->pipe;
 	int plane = intel_plane->plane;
+	u32 start_vbl_count;
+	bool atomic_update;
+
+	atomic_update = intel_pipe_update_start(intel_crtc, &start_vbl_count);
 
 	I915_WRITE(SPCNTR(pipe, plane), I915_READ(SPCNTR(pipe, plane)) &
 		   ~SP_ENABLE);
@@ -161,6 +257,9 @@ vlv_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc)
 	I915_WRITE(SPSURF(pipe, plane), 0);
 	POSTING_READ(SPSURF(pipe, plane));
 
+	if (atomic_update)
+		intel_pipe_update_end(intel_crtc, start_vbl_count);
+
 	intel_update_sprite_watermarks(dplane, crtc, 0, 0, false, false);  }
 
@@ -226,10 +325,13 @@ ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 	struct drm_device *dev = plane->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_plane *intel_plane = to_intel_plane(plane);
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	int pipe = intel_plane->pipe;
 	u32 sprctl, sprscale = 0;
 	unsigned long sprsurf_offset, linear_offset;
 	int pixel_size = drm_format_plane_cpp(fb->pixel_format, 0);
+	u32 start_vbl_count;
+	bool atomic_update;
 
 	sprctl = I915_READ(SPRCTL(pipe));
 
@@ -299,6 +401,8 @@ ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 					       pixel_size, fb->pitches[0]);
 	linear_offset -= sprsurf_offset;
 
+	atomic_update = intel_pipe_update_start(intel_crtc, &start_vbl_count);
+
 	I915_WRITE(SPRSTRIDE(pipe), fb->pitches[0]);
 	I915_WRITE(SPRPOS(pipe), (crtc_y << 16) | crtc_x);
 
@@ -318,6 +422,9 @@ ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 	I915_WRITE(SPRSURF(pipe),
 		   i915_gem_obj_ggtt_offset(obj) + sprsurf_offset);
 	POSTING_READ(SPRSURF(pipe));
+
+	if (atomic_update)
+		intel_pipe_update_end(intel_crtc, start_vbl_count);
 }
 
 static void
@@ -326,7 +433,12 @@ ivb_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
 	struct drm_device *dev = plane->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_plane *intel_plane = to_intel_plane(plane);
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	int pipe = intel_plane->pipe;
+	u32 start_vbl_count;
+	bool atomic_update;
+
+	atomic_update = intel_pipe_update_start(intel_crtc, &start_vbl_count);
 
 	I915_WRITE(SPRCTL(pipe), I915_READ(SPRCTL(pipe)) & ~SPRITE_ENABLE);
 	/* Can't leave the scaler enabled... */ @@ -336,6 +448,9 @@ ivb_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
 	I915_WRITE(SPRSURF(pipe), 0);
 	POSTING_READ(SPRSURF(pipe));
 
+	if (atomic_update)
+		intel_pipe_update_end(intel_crtc, start_vbl_count);
+
 	/*
 	 * Avoid underruns when disabling the sprite.
 	 * FIXME remove once watermark updates are done properly.
@@ -410,10 +525,13 @@ ilk_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 	struct drm_device *dev = plane->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_plane *intel_plane = to_intel_plane(plane);
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	int pipe = intel_plane->pipe;
 	unsigned long dvssurf_offset, linear_offset;
 	u32 dvscntr, dvsscale;
 	int pixel_size = drm_format_plane_cpp(fb->pixel_format, 0);
+	u32 start_vbl_count;
+	bool atomic_update;
 
 	dvscntr = I915_READ(DVSCNTR(pipe));
 
@@ -478,6 +596,8 @@ ilk_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 					       pixel_size, fb->pitches[0]);
 	linear_offset -= dvssurf_offset;
 
+	atomic_update = intel_pipe_update_start(intel_crtc, &start_vbl_count);
+
 	I915_WRITE(DVSSTRIDE(pipe), fb->pitches[0]);
 	I915_WRITE(DVSPOS(pipe), (crtc_y << 16) | crtc_x);
 
@@ -492,6 +612,9 @@ ilk_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 	I915_WRITE(DVSSURF(pipe),
 		   i915_gem_obj_ggtt_offset(obj) + dvssurf_offset);
 	POSTING_READ(DVSSURF(pipe));
+
+	if (atomic_update)
+		intel_pipe_update_end(intel_crtc, start_vbl_count);
 }
 
 static void
@@ -500,7 +623,12 @@ ilk_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
 	struct drm_device *dev = plane->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_plane *intel_plane = to_intel_plane(plane);
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	int pipe = intel_plane->pipe;
+	u32 start_vbl_count;
+	bool atomic_update;
+
+	atomic_update = intel_pipe_update_start(intel_crtc, &start_vbl_count);
 
 	I915_WRITE(DVSCNTR(pipe), I915_READ(DVSCNTR(pipe)) & ~DVS_ENABLE);
 	/* Disable the scaler */
@@ -509,6 +637,9 @@ ilk_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
 	I915_WRITE(DVSSURF(pipe), 0);
 	POSTING_READ(DVSSURF(pipe));
 
+	if (atomic_update)
+		intel_pipe_update_end(intel_crtc, start_vbl_count);
+
 	/*
 	 * Avoid underruns when disabling the sprite.
 	 * FIXME remove once watermark updates are done properly.
--
1.8.3.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v8 3/9] drm/i915: Make sprite updates atomic

[Murthy, Arun R]

> Add a mechanism by which we can evade the leading edge of vblank. This
> guarantees that no two sprite register writes will straddle on either
> side of the vblank start, and that means all the writes will be latched
> together in one atomic operation.

Here only one sprite update followed by the primary enable/disable can be
achieved atomically. But I feel update of all planes are to be 
considered, i.e
update of planes per pipe basis to achieve atomicity.
>
> We do the vblank evade by checking the scanline counter, and if it's too
> close to the start of vblank (too close has been hardcoded to 100usec
> for now), we will wait for the vblank start to pass. In order to
> eliminate random delayes from the rest of the system, we operate with
> interrupts disabled, except when waiting for the vblank obviously.

This can be achieved easily by checking the previous vblank time in
drm_get_last_vblanktimestamp(), using this the next vblank time cab be
predicted. Using these with the hardcoded value 100usec, a check can be
made to continue or to wait for a vblank. For waiting for a vblank instead
of adding new function just use the available intel_wait_for_vblank().

last_vblank = drm_get_last_vblanktimestamp();
curr_time = do_getttimeofday();
if (curr_time.tv_usec - (last_vblank +  VBLANK_TIME_INTERVAL) > 100)
     /* acquire lock and proceed*/
else
     /* wait for one vblank, acquire lock and proceed */

Thanks and Regards,
Arun R Murthy
--------------------

[PATCH v8 3/9] drm/i915: Make sprite updates atomic

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Add a mechanism by which we can evade the leading edge of vblank. This
guarantees that no two sprite register writes will straddle on either
side of the vblank start, and that means all the writes will be latched
together in one atomic operation.

We do the vblank evade by checking the scanline counter, and if it's too
close to the start of vblank (too close has been hardcoded to 100usec
for now), we will wait for the vblank start to pass. In order to
eliminate random delayes from the rest of the system, we operate with
interrupts disabled, except when waiting for the vblank obviously.

Note that we now go digging through pipe_to_crtc_mapping[] in the
vblank interrupt handler, which is a bit dangerous since we set up
interrupts before the crtcs. However in this case since it's the vblank
interrupt, we don't actually unmask it until some piece of code
requests it.

v2: preempt_check_resched() calls after local_irq_enable() (Jesse)
    Hook up the vblank irq stuff on BDW as well
v3: Pass intel_crtc instead of drm_crtc (Daniel)
    Warn if crtc.mutex isn't locked (Daniel)
    Add an explicit compiler barrier and document the barriers (Daniel)
    Note the irq vs. modeset setup madness in the commit message (Daniel)
v4: Use prepare_to_wait() & co. directly and eliminate vbl_received
v5: Refactor intel_pipe_handle_vblank() vs. drm_handle_vblank() (Chris)
    Check for min/max scanline <= 0 (Chris)
    Don't call intel_pipe_update_end() if start failed totally (Chris)
    Check that the vblank counters match on both sides of the critical
    section (Chris)
v6: Fix atomic update for interlaced modes
v7: Reorder code for better readability (Chris)
v8: Drop preempt_check_resched(). It's not available to modules
    anymore and isn't even needed unless we ourselves cause
    a wakeup needing reschedule while interrupts are off

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
Reviewed-by: Sourab Gupta <sourabgupta@gmail.com>
Reviewed-by: Akash Goel <akash.goels@gmail.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c      |  25 +++++--
 drivers/gpu/drm/i915/intel_display.c |   2 +
 drivers/gpu/drm/i915/intel_drv.h     |   2 +
 drivers/gpu/drm/i915/intel_sprite.c  | 131 +++++++++++++++++++++++++++++++++++
 4 files changed, 154 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index ed30a5e..7e0d577 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1556,6 +1556,19 @@ static void gen6_rps_irq_handler(struct drm_i915_private *dev_priv, u32 pm_iir)
 	}
 }
 
+static bool intel_pipe_handle_vblank(struct drm_device *dev, enum pipe pipe)
+{
+	struct intel_crtc *crtc;
+
+	if (!drm_handle_vblank(dev, pipe))
+		return false;
+
+	crtc = to_intel_crtc(intel_get_crtc_for_pipe(dev, pipe));
+	wake_up(&crtc->vbl_wait);
+
+	return true;
+}
+
 static void valleyview_pipestat_irq_handler(struct drm_device *dev, u32 iir)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -1607,7 +1620,7 @@ static void valleyview_pipestat_irq_handler(struct drm_device *dev, u32 iir)
 
 	for_each_pipe(pipe) {
 		if (pipe_stats[pipe] & PIPE_START_VBLANK_INTERRUPT_STATUS)
-			drm_handle_vblank(dev, pipe);
+			intel_pipe_handle_vblank(dev, pipe);
 
 		if (pipe_stats[pipe] & PLANE_FLIP_DONE_INT_STATUS_VLV) {
 			intel_prepare_page_flip(dev, pipe);
@@ -1850,7 +1863,7 @@ static void ilk_display_irq_handler(struct drm_device *dev, u32 de_iir)
 
 	for_each_pipe(pipe) {
 		if (de_iir & DE_PIPE_VBLANK(pipe))
-			drm_handle_vblank(dev, pipe);
+			intel_pipe_handle_vblank(dev, pipe);
 
 		if (de_iir & DE_PIPE_FIFO_UNDERRUN(pipe))
 			if (intel_set_cpu_fifo_underrun_reporting(dev, pipe, false))
@@ -1900,7 +1913,7 @@ static void ivb_display_irq_handler(struct drm_device *dev, u32 de_iir)
 
 	for_each_pipe(pipe) {
 		if (de_iir & (DE_PIPE_VBLANK_IVB(pipe)))
-			drm_handle_vblank(dev, pipe);
+			intel_pipe_handle_vblank(dev, pipe);
 
 		/* plane/pipes map 1:1 on ilk+ */
 		if (de_iir & DE_PLANE_FLIP_DONE_IVB(pipe)) {
@@ -2043,7 +2056,7 @@ static irqreturn_t gen8_irq_handler(int irq, void *arg)
 
 		pipe_iir = I915_READ(GEN8_DE_PIPE_IIR(pipe));
 		if (pipe_iir & GEN8_PIPE_VBLANK)
-			drm_handle_vblank(dev, pipe);
+			intel_pipe_handle_vblank(dev, pipe);
 
 		if (pipe_iir & GEN8_PIPE_PRIMARY_FLIP_DONE) {
 			intel_prepare_page_flip(dev, pipe);
@@ -3391,7 +3404,7 @@ static bool i8xx_handle_vblank(struct drm_device *dev,
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	u16 flip_pending = DISPLAY_PLANE_FLIP_PENDING(plane);
 
-	if (!drm_handle_vblank(dev, pipe))
+	if (!intel_pipe_handle_vblank(dev, pipe))
 		return false;
 
 	if ((iir & flip_pending) == 0)
@@ -3576,7 +3589,7 @@ static bool i915_handle_vblank(struct drm_device *dev,
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	u32 flip_pending = DISPLAY_PLANE_FLIP_PENDING(plane);
 
-	if (!drm_handle_vblank(dev, pipe))
+	if (!intel_pipe_handle_vblank(dev, pipe))
 		return false;
 
 	if ((iir & flip_pending) == 0)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index ca33ef5..993597b 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -10566,6 +10566,8 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
 		intel_crtc->plane = !pipe;
 	}
 
+	init_waitqueue_head(&intel_crtc->vbl_wait);
+
 	BUG_ON(pipe >= ARRAY_SIZE(dev_priv->plane_to_crtc_mapping) ||
 	       dev_priv->plane_to_crtc_mapping[intel_crtc->plane] != NULL);
 	dev_priv->plane_to_crtc_mapping[intel_crtc->plane] = &intel_crtc->base;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 96ae78d..d8b540b 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -401,6 +401,8 @@ struct intel_crtc {
 		/* watermarks currently being used  */
 		struct intel_pipe_wm active;
 	} wm;
+
+	wait_queue_head_t vbl_wait;
 };
 
 struct intel_plane_wm_parameters {
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 336ae6c..a3ed840 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -37,6 +37,89 @@
 #include <drm/i915_drm.h>
 #include "i915_drv.h"
 
+static int usecs_to_scanlines(const struct drm_display_mode *mode, int usecs)
+{
+	/* paranoia */
+	if (!mode->crtc_htotal)
+		return 1;
+
+	return DIV_ROUND_UP(usecs * mode->crtc_clock, 1000 * mode->crtc_htotal);
+}
+
+static bool intel_pipe_update_start(struct intel_crtc *crtc, uint32_t *start_vbl_count)
+{
+	struct drm_device *dev = crtc->base.dev;
+	const struct drm_display_mode *mode = &crtc->config.adjusted_mode;
+	enum pipe pipe = crtc->pipe;
+	long timeout = msecs_to_jiffies_timeout(1);
+	int scanline, min, max, vblank_start;
+	DEFINE_WAIT(wait);
+
+	WARN_ON(!mutex_is_locked(&crtc->base.mutex));
+
+	vblank_start = mode->crtc_vblank_start;
+	if (mode->flags & DRM_MODE_FLAG_INTERLACE)
+		vblank_start = DIV_ROUND_UP(vblank_start, 2);
+
+	/* FIXME needs to be calibrated sensibly */
+	min = vblank_start - usecs_to_scanlines(mode, 100);
+	max = vblank_start - 1;
+
+	if (min <= 0 || max <= 0)
+		return false;
+
+	if (WARN_ON(drm_vblank_get(dev, pipe)))
+		return false;
+
+	local_irq_disable();
+
+	for (;;) {
+		/*
+		 * prepare_to_wait() has a memory barrier, which guarantees
+		 * other CPUs can see the task state update by the time we
+		 * read the scanline.
+		 */
+		prepare_to_wait(&crtc->vbl_wait, &wait, TASK_UNINTERRUPTIBLE);
+
+		scanline = intel_get_crtc_scanline(crtc);
+		if (scanline < min || scanline > max)
+			break;
+
+		if (timeout <= 0) {
+			DRM_ERROR("Potential atomic update failure on pipe %c\n",
+				  pipe_name(crtc->pipe));
+			break;
+		}
+
+		local_irq_enable();
+
+		timeout = schedule_timeout(timeout);
+
+		local_irq_disable();
+	}
+
+	finish_wait(&crtc->vbl_wait, &wait);
+
+	drm_vblank_put(dev, pipe);
+
+	*start_vbl_count = dev->driver->get_vblank_counter(dev, pipe);
+
+	return true;
+}
+
+static void intel_pipe_update_end(struct intel_crtc *crtc, u32 start_vbl_count)
+{
+	struct drm_device *dev = crtc->base.dev;
+	enum pipe pipe = crtc->pipe;
+	u32 end_vbl_count = dev->driver->get_vblank_counter(dev, pipe);
+
+	local_irq_enable();
+
+	if (start_vbl_count != end_vbl_count)
+		DRM_ERROR("Atomic update failure on pipe %c (start=%u end=%u)\n",
+			  pipe_name(pipe), start_vbl_count, end_vbl_count);
+}
+
 static void
 vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
 		 struct drm_framebuffer *fb,
@@ -48,11 +131,14 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
 	struct drm_device *dev = dplane->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_plane *intel_plane = to_intel_plane(dplane);
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	int pipe = intel_plane->pipe;
 	int plane = intel_plane->plane;
 	u32 sprctl;
 	unsigned long sprsurf_offset, linear_offset;
 	int pixel_size = drm_format_plane_cpp(fb->pixel_format, 0);
+	u32 start_vbl_count;
+	bool atomic_update;
 
 	sprctl = I915_READ(SPCNTR(pipe, plane));
 
@@ -131,6 +217,8 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
 							fb->pitches[0]);
 	linear_offset -= sprsurf_offset;
 
+	atomic_update = intel_pipe_update_start(intel_crtc, &start_vbl_count);
+
 	I915_WRITE(SPSTRIDE(pipe, plane), fb->pitches[0]);
 	I915_WRITE(SPPOS(pipe, plane), (crtc_y << 16) | crtc_x);
 
@@ -144,6 +232,9 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
 	I915_WRITE(SPSURF(pipe, plane), i915_gem_obj_ggtt_offset(obj) +
 		   sprsurf_offset);
 	POSTING_READ(SPSURF(pipe, plane));
+
+	if (atomic_update)
+		intel_pipe_update_end(intel_crtc, start_vbl_count);
 }
 
 static void
@@ -152,8 +243,13 @@ vlv_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc)
 	struct drm_device *dev = dplane->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_plane *intel_plane = to_intel_plane(dplane);
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	int pipe = intel_plane->pipe;
 	int plane = intel_plane->plane;
+	u32 start_vbl_count;
+	bool atomic_update;
+
+	atomic_update = intel_pipe_update_start(intel_crtc, &start_vbl_count);
 
 	I915_WRITE(SPCNTR(pipe, plane), I915_READ(SPCNTR(pipe, plane)) &
 		   ~SP_ENABLE);
@@ -161,6 +257,9 @@ vlv_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc)
 	I915_WRITE(SPSURF(pipe, plane), 0);
 	POSTING_READ(SPSURF(pipe, plane));
 
+	if (atomic_update)
+		intel_pipe_update_end(intel_crtc, start_vbl_count);
+
 	intel_update_sprite_watermarks(dplane, crtc, 0, 0, false, false);
 }
 
@@ -226,10 +325,13 @@ ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 	struct drm_device *dev = plane->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_plane *intel_plane = to_intel_plane(plane);
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	int pipe = intel_plane->pipe;
 	u32 sprctl, sprscale = 0;
 	unsigned long sprsurf_offset, linear_offset;
 	int pixel_size = drm_format_plane_cpp(fb->pixel_format, 0);
+	u32 start_vbl_count;
+	bool atomic_update;
 
 	sprctl = I915_READ(SPRCTL(pipe));
 
@@ -299,6 +401,8 @@ ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 					       pixel_size, fb->pitches[0]);
 	linear_offset -= sprsurf_offset;
 
+	atomic_update = intel_pipe_update_start(intel_crtc, &start_vbl_count);
+
 	I915_WRITE(SPRSTRIDE(pipe), fb->pitches[0]);
 	I915_WRITE(SPRPOS(pipe), (crtc_y << 16) | crtc_x);
 
@@ -318,6 +422,9 @@ ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 	I915_WRITE(SPRSURF(pipe),
 		   i915_gem_obj_ggtt_offset(obj) + sprsurf_offset);
 	POSTING_READ(SPRSURF(pipe));
+
+	if (atomic_update)
+		intel_pipe_update_end(intel_crtc, start_vbl_count);
 }
 
 static void
@@ -326,7 +433,12 @@ ivb_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
 	struct drm_device *dev = plane->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_plane *intel_plane = to_intel_plane(plane);
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	int pipe = intel_plane->pipe;
+	u32 start_vbl_count;
+	bool atomic_update;
+
+	atomic_update = intel_pipe_update_start(intel_crtc, &start_vbl_count);
 
 	I915_WRITE(SPRCTL(pipe), I915_READ(SPRCTL(pipe)) & ~SPRITE_ENABLE);
 	/* Can't leave the scaler enabled... */
@@ -336,6 +448,9 @@ ivb_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
 	I915_WRITE(SPRSURF(pipe), 0);
 	POSTING_READ(SPRSURF(pipe));
 
+	if (atomic_update)
+		intel_pipe_update_end(intel_crtc, start_vbl_count);
+
 	/*
 	 * Avoid underruns when disabling the sprite.
 	 * FIXME remove once watermark updates are done properly.
@@ -410,10 +525,13 @@ ilk_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 	struct drm_device *dev = plane->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_plane *intel_plane = to_intel_plane(plane);
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	int pipe = intel_plane->pipe;
 	unsigned long dvssurf_offset, linear_offset;
 	u32 dvscntr, dvsscale;
 	int pixel_size = drm_format_plane_cpp(fb->pixel_format, 0);
+	u32 start_vbl_count;
+	bool atomic_update;
 
 	dvscntr = I915_READ(DVSCNTR(pipe));
 
@@ -478,6 +596,8 @@ ilk_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 					       pixel_size, fb->pitches[0]);
 	linear_offset -= dvssurf_offset;
 
+	atomic_update = intel_pipe_update_start(intel_crtc, &start_vbl_count);
+
 	I915_WRITE(DVSSTRIDE(pipe), fb->pitches[0]);
 	I915_WRITE(DVSPOS(pipe), (crtc_y << 16) | crtc_x);
 
@@ -492,6 +612,9 @@ ilk_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 	I915_WRITE(DVSSURF(pipe),
 		   i915_gem_obj_ggtt_offset(obj) + dvssurf_offset);
 	POSTING_READ(DVSSURF(pipe));
+
+	if (atomic_update)
+		intel_pipe_update_end(intel_crtc, start_vbl_count);
 }
 
 static void
@@ -500,7 +623,12 @@ ilk_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
 	struct drm_device *dev = plane->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_plane *intel_plane = to_intel_plane(plane);
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	int pipe = intel_plane->pipe;
+	u32 start_vbl_count;
+	bool atomic_update;
+
+	atomic_update = intel_pipe_update_start(intel_crtc, &start_vbl_count);
 
 	I915_WRITE(DVSCNTR(pipe), I915_READ(DVSCNTR(pipe)) & ~DVS_ENABLE);
 	/* Disable the scaler */
@@ -509,6 +637,9 @@ ilk_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
 	I915_WRITE(DVSSURF(pipe), 0);
 	POSTING_READ(DVSSURF(pipe));
 
+	if (atomic_update)
+		intel_pipe_update_end(intel_crtc, start_vbl_count);
+
 	/*
 	 * Avoid underruns when disabling the sprite.
 	 * FIXME remove once watermark updates are done properly.
-- 
1.8.3.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx