[PATCH 1/3] mx3fb: do not support panning with fb blanked

[PATCH 1/3] mx3fb: do not support panning with fb blanked

[Liu Ying]

This patch checks if framebuffer is unblanked before
we actually trigger panning in custom pan display
function.

Signed-off-by: Liu Ying <Ying.Liu@freescale.com>
---
 drivers/video/mx3fb.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/drivers/video/mx3fb.c b/drivers/video/mx3fb.c
index e3406ab..d53db60 100644
--- a/drivers/video/mx3fb.c
+++ b/drivers/video/mx3fb.c
@@ -1063,6 +1063,11 @@ static int mx3fb_pan_display(struct fb_var_screeninfo *var,
 	dev_dbg(fbi->device, "%s [%c]\n", __func__,
 		list_empty(&mx3_fbi->idmac_channel->queue) ? '-' : '+');
 
+	if (mx3_fbi->blank != FB_BLANK_UNBLANK) {
+		dev_dbg(fbi->device, "panning with fb blanked not supported\n");
+		return -EFAULT;
+	}
+
 	if (var->xoffset > 0) {
 		dev_dbg(fbi->device, "x panning not supported\n");
 		return -EINVAL;
-- 
1.7.1

[PATCH 2/3] mx3fb: support pan display with fb_set_var

[Liu Ying]

Users may call FBIOPUT_VSCREENINFO ioctrl to do pan display.
This ioctrl relies on fb_set_var() to do the job. fb_set_var()
calls the custom fb_set_par() method and then calls the custom
fb_pan_display() method. Before calling the custom fb_pan_display()
method, info->var is already updated from the new *var in fb_set_var().
And, the custom fb_pan_display() method checks if xoffset and yoffset
in info->var and the new *var are different before doing actual panning,
which prevents the panning from happening within fb_set_var() context.
This patch caches the current var info locally in mx3fb driver so that
pan display with fb_set_var is supported.

Signed-off-by: Liu Ying <Ying.Liu@freescale.com>
---
 drivers/video/mx3fb.c |   33 ++++++++++++++++++++++++++-------
 1 files changed, 26 insertions(+), 7 deletions(-)

diff --git a/drivers/video/mx3fb.c b/drivers/video/mx3fb.c
index d53db60..2dd11c4 100644
--- a/drivers/video/mx3fb.c
+++ b/drivers/video/mx3fb.c
@@ -268,7 +268,7 @@ struct mx3fb_info {
 	dma_cookie_t			cookie;
 	struct scatterlist		sg[2];
 
-	u32				sync;	/* preserve var->sync flags */
+	struct fb_var_screeninfo	cur_var; /* current var info */
 };
 
 static void mx3fb_dma_done(void *);
@@ -723,7 +723,7 @@ static void mx3fb_dma_done(void *arg)
 
 static int __set_par(struct fb_info *fbi, bool lock)
 {
-	u32 mem_len;
+	u32 mem_len, cur_xoffset, cur_yoffset;
 	struct ipu_di_signal_cfg sig_cfg;
 	enum ipu_panel mode = IPU_PANEL_TFT;
 	struct mx3fb_info *mx3_fbi = fbi->par;
@@ -805,8 +805,25 @@ static int __set_par(struct fb_info *fbi, bool lock)
 	video->out_height	= fbi->var.yres;
 	video->out_stride	= fbi->var.xres_virtual;
 
-	if (mx3_fbi->blank = FB_BLANK_UNBLANK)
+	if (mx3_fbi->blank = FB_BLANK_UNBLANK) {
 		sdc_enable_channel(mx3_fbi);
+		/*
+		 * sg[0] points to fb smem_start address
+		 * and is actually active in controller.
+		 */
+		mx3_fbi->cur_var.xoffset = 0;
+		mx3_fbi->cur_var.yoffset = 0;
+	}
+
+	/*
+	 * Preserve xoffset and yoffest in case they are
+	 * inactive in controller as fb is blanked.
+	 */
+	cur_xoffset = mx3_fbi->cur_var.xoffset;
+	cur_yoffset = mx3_fbi->cur_var.yoffset;
+	mx3_fbi->cur_var = fbi->var;
+	mx3_fbi->cur_var.xoffset = cur_xoffset;
+	mx3_fbi->cur_var.yoffset = cur_yoffset;
 
 	return 0;
 }
@@ -926,8 +943,8 @@ static int mx3fb_check_var(struct fb_var_screeninfo *var, struct fb_info *fbi)
 	var->grayscale = 0;
 
 	/* Preserve sync flags */
-	var->sync |= mx3_fbi->sync;
-	mx3_fbi->sync |= var->sync;
+	var->sync |= mx3_fbi->cur_var.sync;
+	mx3_fbi->cur_var.sync |= var->sync;
 
 	return 0;
 }
@@ -1073,8 +1090,8 @@ static int mx3fb_pan_display(struct fb_var_screeninfo *var,
 		return -EINVAL;
 	}
 
-	if (fbi->var.xoffset = var->xoffset &&
-	    fbi->var.yoffset = var->yoffset)
+	if (mx3_fbi->cur_var.xoffset = var->xoffset &&
+	    mx3_fbi->cur_var.yoffset = var->yoffset)
 		return 0;	/* No change, do nothing */
 
 	y_bottom = var->yoffset;
@@ -1157,6 +1174,8 @@ static int mx3fb_pan_display(struct fb_var_screeninfo *var,
 	else
 		fbi->var.vmode &= ~FB_VMODE_YWRAP;
 
+	mx3_fbi->cur_var = fbi->var;
+
 	mutex_unlock(&mx3_fbi->mutex);
 
 	dev_dbg(fbi->device, "Update complete\n");
-- 
1.7.1

Re: [PATCH 2/3] mx3fb: support pan display with fb_set_var

[Florian Tobias Schandinat]

On 06/11/2012 01:06 AM, Liu Ying wrote:
> Users may call FBIOPUT_VSCREENINFO ioctrl to do pan display.
> This ioctrl relies on fb_set_var() to do the job. fb_set_var()
> calls the custom fb_set_par() method and then calls the custom
> fb_pan_display() method. Before calling the custom fb_pan_display()
> method, info->var is already updated from the new *var in fb_set_var().
> And, the custom fb_pan_display() method checks if xoffset and yoffset
> in info->var and the new *var are different before doing actual panning,
> which prevents the panning from happening within fb_set_var() context.
> This patch caches the current var info locally in mx3fb driver so that
> pan display with fb_set_var is supported.
> 
> Signed-off-by: Liu Ying <Ying.Liu@freescale.com>

Applied patches 2 and 3 of this series.


Thanks,

Florian Tobias Schandinat

> ---
>  drivers/video/mx3fb.c |   33 ++++++++++++++++++++++++++-------
>  1 files changed, 26 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/video/mx3fb.c b/drivers/video/mx3fb.c
> index d53db60..2dd11c4 100644
> --- a/drivers/video/mx3fb.c
> +++ b/drivers/video/mx3fb.c
> @@ -268,7 +268,7 @@ struct mx3fb_info {
>  	dma_cookie_t			cookie;
>  	struct scatterlist		sg[2];
>  
> -	u32				sync;	/* preserve var->sync flags */
> +	struct fb_var_screeninfo	cur_var; /* current var info */
>  };
>  
>  static void mx3fb_dma_done(void *);
> @@ -723,7 +723,7 @@ static void mx3fb_dma_done(void *arg)
>  
>  static int __set_par(struct fb_info *fbi, bool lock)
>  {
> -	u32 mem_len;
> +	u32 mem_len, cur_xoffset, cur_yoffset;
>  	struct ipu_di_signal_cfg sig_cfg;
>  	enum ipu_panel mode = IPU_PANEL_TFT;
>  	struct mx3fb_info *mx3_fbi = fbi->par;
> @@ -805,8 +805,25 @@ static int __set_par(struct fb_info *fbi, bool lock)
>  	video->out_height	= fbi->var.yres;
>  	video->out_stride	= fbi->var.xres_virtual;
>  
> -	if (mx3_fbi->blank = FB_BLANK_UNBLANK)
> +	if (mx3_fbi->blank = FB_BLANK_UNBLANK) {
>  		sdc_enable_channel(mx3_fbi);
> +		/*
> +		 * sg[0] points to fb smem_start address
> +		 * and is actually active in controller.
> +		 */
> +		mx3_fbi->cur_var.xoffset = 0;
> +		mx3_fbi->cur_var.yoffset = 0;
> +	}
> +
> +	/*
> +	 * Preserve xoffset and yoffest in case they are
> +	 * inactive in controller as fb is blanked.
> +	 */
> +	cur_xoffset = mx3_fbi->cur_var.xoffset;
> +	cur_yoffset = mx3_fbi->cur_var.yoffset;
> +	mx3_fbi->cur_var = fbi->var;
> +	mx3_fbi->cur_var.xoffset = cur_xoffset;
> +	mx3_fbi->cur_var.yoffset = cur_yoffset;
>  
>  	return 0;
>  }
> @@ -926,8 +943,8 @@ static int mx3fb_check_var(struct fb_var_screeninfo *var, struct fb_info *fbi)
>  	var->grayscale = 0;
>  
>  	/* Preserve sync flags */
> -	var->sync |= mx3_fbi->sync;
> -	mx3_fbi->sync |= var->sync;
> +	var->sync |= mx3_fbi->cur_var.sync;
> +	mx3_fbi->cur_var.sync |= var->sync;
>  
>  	return 0;
>  }
> @@ -1073,8 +1090,8 @@ static int mx3fb_pan_display(struct fb_var_screeninfo *var,
>  		return -EINVAL;
>  	}
>  
> -	if (fbi->var.xoffset = var->xoffset &&
> -	    fbi->var.yoffset = var->yoffset)
> +	if (mx3_fbi->cur_var.xoffset = var->xoffset &&
> +	    mx3_fbi->cur_var.yoffset = var->yoffset)
>  		return 0;	/* No change, do nothing */
>  
>  	y_bottom = var->yoffset;
> @@ -1157,6 +1174,8 @@ static int mx3fb_pan_display(struct fb_var_screeninfo *var,
>  	else
>  		fbi->var.vmode &= ~FB_VMODE_YWRAP;
>  
> +	mx3_fbi->cur_var = fbi->var;
> +
>  	mutex_unlock(&mx3_fbi->mutex);
>  
>  	dev_dbg(fbi->device, "Update complete\n");

[PATCH 3/3] mx3fb: avoid screen flash when panning with fb_set_var

[Liu Ying]

Users may call FBIOPUT_VSCREENINFO ioctrl to do pan display.
This ioctrl relies on fb_set_var() to do the job. fb_set_var()
calls custom fb_set_par() method and then calls custom
fb_pan_display() method. The current implementation of mx3fb
reinitializes IPU display controller every time the custom
fb_set_par() method is called, which makes the screen flash
if fb_set_var() is called to do panning frequently. This patch
compares the new var info with the cached old one to decide
whether we really need to reinitialize IPU display controller.
We ignore xoffset and yoffset update because it doesn't require
to reinitialize the controller. Users may specify activate field
of var info with FB_ACTIVATE_NOW and FB_ACTIVATE_FORCE to
reinialize the controller by force.

Signed-off-by: Liu Ying <Ying.Liu@freescale.com>
---
 drivers/video/mx3fb.c |   22 +++++++++++++++++++++-
 1 files changed, 21 insertions(+), 1 deletions(-)

diff --git a/drivers/video/mx3fb.c b/drivers/video/mx3fb.c
index 2dd11c4..7d0aa7b 100644
--- a/drivers/video/mx3fb.c
+++ b/drivers/video/mx3fb.c
@@ -721,6 +721,26 @@ static void mx3fb_dma_done(void *arg)
 	complete(&mx3_fbi->flip_cmpl);
 }
 
+static bool mx3fb_must_set_par(struct fb_info *fbi)
+{
+	struct mx3fb_info *mx3_fbi = fbi->par;
+	struct fb_var_screeninfo old_var = mx3_fbi->cur_var;
+	struct fb_var_screeninfo new_var = fbi->var;
+
+	if ((fbi->var.activate & FB_ACTIVATE_FORCE) &&
+	    (fbi->var.activate & FB_ACTIVATE_MASK) = FB_ACTIVATE_NOW)
+		return true;
+
+	/*
+	 * Ignore xoffset and yoffset update,
+	 * because pan display handles this case.
+	 */
+	old_var.xoffset = new_var.xoffset;
+	old_var.yoffset = new_var.yoffset;
+
+	return !!memcmp(&old_var, &new_var, sizeof(struct fb_var_screeninfo));
+}
+
 static int __set_par(struct fb_info *fbi, bool lock)
 {
 	u32 mem_len, cur_xoffset, cur_yoffset;
@@ -844,7 +864,7 @@ static int mx3fb_set_par(struct fb_info *fbi)
 
 	mutex_lock(&mx3_fbi->mutex);
 
-	ret = __set_par(fbi, true);
+	ret = mx3fb_must_set_par(fbi) ? __set_par(fbi, true) : 0;
 
 	mutex_unlock(&mx3_fbi->mutex);
 
-- 
1.7.1

Re: [PATCH 3/3] mx3fb: avoid screen flash when panning with fb_set_var

[Liu Ying]

Hi, Guennadi,

Could you please review the second(2/3) patch and the third(3/3) patch
of this series?
I have addressed your comments you gave in the previous review cycle.

Thanks.

2012/6/11, Liu Ying <Ying.Liu@freescale.com>:
> Users may call FBIOPUT_VSCREENINFO ioctrl to do pan display.
> This ioctrl relies on fb_set_var() to do the job. fb_set_var()
> calls custom fb_set_par() method and then calls custom
> fb_pan_display() method. The current implementation of mx3fb
> reinitializes IPU display controller every time the custom
> fb_set_par() method is called, which makes the screen flash
> if fb_set_var() is called to do panning frequently. This patch
> compares the new var info with the cached old one to decide
> whether we really need to reinitialize IPU display controller.
> We ignore xoffset and yoffset update because it doesn't require
> to reinitialize the controller. Users may specify activate field
> of var info with FB_ACTIVATE_NOW and FB_ACTIVATE_FORCE to
> reinialize the controller by force.
>
> Signed-off-by: Liu Ying <Ying.Liu@freescale.com>
> ---
>  drivers/video/mx3fb.c |   22 +++++++++++++++++++++-
>  1 files changed, 21 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/video/mx3fb.c b/drivers/video/mx3fb.c
> index 2dd11c4..7d0aa7b 100644
> --- a/drivers/video/mx3fb.c
> +++ b/drivers/video/mx3fb.c
> @@ -721,6 +721,26 @@ static void mx3fb_dma_done(void *arg)
>  	complete(&mx3_fbi->flip_cmpl);
>  }
>
> +static bool mx3fb_must_set_par(struct fb_info *fbi)
> +{
> +	struct mx3fb_info *mx3_fbi = fbi->par;
> +	struct fb_var_screeninfo old_var = mx3_fbi->cur_var;
> +	struct fb_var_screeninfo new_var = fbi->var;
> +
> +	if ((fbi->var.activate & FB_ACTIVATE_FORCE) &&
> +	    (fbi->var.activate & FB_ACTIVATE_MASK) = FB_ACTIVATE_NOW)
> +		return true;
> +
> +	/*
> +	 * Ignore xoffset and yoffset update,
> +	 * because pan display handles this case.
> +	 */
> +	old_var.xoffset = new_var.xoffset;
> +	old_var.yoffset = new_var.yoffset;
> +
> +	return !!memcmp(&old_var, &new_var, sizeof(struct fb_var_screeninfo));
> +}
> +
>  static int __set_par(struct fb_info *fbi, bool lock)
>  {
>  	u32 mem_len, cur_xoffset, cur_yoffset;
> @@ -844,7 +864,7 @@ static int mx3fb_set_par(struct fb_info *fbi)
>
>  	mutex_lock(&mx3_fbi->mutex);
>
> -	ret = __set_par(fbi, true);
> +	ret = mx3fb_must_set_par(fbi) ? __set_par(fbi, true) : 0;
>
>  	mutex_unlock(&mx3_fbi->mutex);
>
> --
> 1.7.1
>
>
>


-- 
Best Regards,
Liu Ying

Re: [PATCH 1/3] mx3fb: do not support panning with fb blanked

[Russell King - ARM Linux]

On Mon, Jun 11, 2012 at 09:06:48AM +0800, Liu Ying wrote:
> This patch checks if framebuffer is unblanked before
> we actually trigger panning in custom pan display
> function.
> 
> Signed-off-by: Liu Ying <Ying.Liu@freescale.com>
> ---
>  drivers/video/mx3fb.c |    5 +++++
>  1 files changed, 5 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/video/mx3fb.c b/drivers/video/mx3fb.c
> index e3406ab..d53db60 100644
> --- a/drivers/video/mx3fb.c
> +++ b/drivers/video/mx3fb.c
> @@ -1063,6 +1063,11 @@ static int mx3fb_pan_display(struct fb_var_screeninfo *var,
>  	dev_dbg(fbi->device, "%s [%c]\n", __func__,
>  		list_empty(&mx3_fbi->idmac_channel->queue) ? '-' : '+');
>  
> +	if (mx3_fbi->blank != FB_BLANK_UNBLANK) {
> +		dev_dbg(fbi->device, "panning with fb blanked not supported\n");
> +		return -EFAULT;
> +	}

Why is this an error, and why return -EFAULT?  What userspace access
failed?

Re: [PATCH 1/3] mx3fb: do not support panning with fb blanked

[Liu Ying]

2012/6/11, Russell King - ARM Linux <linux@arm.linux.org.uk>:
> On Mon, Jun 11, 2012 at 09:06:48AM +0800, Liu Ying wrote:
>> This patch checks if framebuffer is unblanked before
>> we actually trigger panning in custom pan display
>> function.
>>
>> Signed-off-by: Liu Ying <Ying.Liu@freescale.com>
>> ---
>>  drivers/video/mx3fb.c |    5 +++++
>>  1 files changed, 5 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/video/mx3fb.c b/drivers/video/mx3fb.c
>> index e3406ab..d53db60 100644
>> --- a/drivers/video/mx3fb.c
>> +++ b/drivers/video/mx3fb.c
>> @@ -1063,6 +1063,11 @@ static int mx3fb_pan_display(struct
>> fb_var_screeninfo *var,
>>  	dev_dbg(fbi->device, "%s [%c]\n", __func__,
>>  		list_empty(&mx3_fbi->idmac_channel->queue) ? '-' : '+');
>>
>> +	if (mx3_fbi->blank != FB_BLANK_UNBLANK) {
>> +		dev_dbg(fbi->device, "panning with fb blanked not supported\n");
>> +		return -EFAULT;
>> +	}
>
> Why is this an error, and why return -EFAULT?  What userspace access
> failed?
>
Hi, Russell,

IMHO, panning with framebuffer blanked is meaningless, at least, it is
not a common usecase. Most users use pan display to swap front buffer
and back buffer when framebuffer is unblanked/active. So, I choose to
take the in question case as an error. Pan display may let user select
a buffer address to be active on display  and I saw the head file
'errno-base.h' comments EFAULT macro to be 'Bad address', so I use
this return value. Perhaps, this return value is not good enough.
Maybe, EIO is better? Would you please help to give some advice on
this?

Thanks.

-- 
Best Regards,
Liu Ying

Re: [PATCH 1/3] mx3fb: do not support panning with fb blanked

[Russell King - ARM Linux]

On Mon, Jun 11, 2012 at 06:46:14PM +0800, Liu Ying wrote:
> 2012/6/11, Russell King - ARM Linux <linux@arm.linux.org.uk>:
> > On Mon, Jun 11, 2012 at 09:06:48AM +0800, Liu Ying wrote:
> >> This patch checks if framebuffer is unblanked before
> >> we actually trigger panning in custom pan display
> >> function.
> >>
> >> Signed-off-by: Liu Ying <Ying.Liu@freescale.com>
> >> ---
> >>  drivers/video/mx3fb.c |    5 +++++
> >>  1 files changed, 5 insertions(+), 0 deletions(-)
> >>
> >> diff --git a/drivers/video/mx3fb.c b/drivers/video/mx3fb.c
> >> index e3406ab..d53db60 100644
> >> --- a/drivers/video/mx3fb.c
> >> +++ b/drivers/video/mx3fb.c
> >> @@ -1063,6 +1063,11 @@ static int mx3fb_pan_display(struct
> >> fb_var_screeninfo *var,
> >>  	dev_dbg(fbi->device, "%s [%c]\n", __func__,
> >>  		list_empty(&mx3_fbi->idmac_channel->queue) ? '-' : '+');
> >>
> >> +	if (mx3_fbi->blank != FB_BLANK_UNBLANK) {
> >> +		dev_dbg(fbi->device, "panning with fb blanked not supported\n");
> >> +		return -EFAULT;
> >> +	}
> >
> > Why is this an error, and why return -EFAULT?  What userspace access
> > failed?
> >
> Hi, Russell,
> 
> IMHO, panning with framebuffer blanked is meaningless, at least, it is
> not a common usecase.

Yes, but why should anything in userspace care whether the display is
blanked or not?

For example, we may pan the display when a user program produces output,
and we're on the last line of the display.  We don't fail that just
because the screen happens to be blank.  So I don't see why we should
actively fail an explicit userspace pan request just because the console
happened to be blanked.

What I'm basically saying is that as far as _userspace_ is concerned,
all the standard APIs should just work as normal whether the display is
blanked or not.

Re: [PATCH 1/3] mx3fb: do not support panning with fb blanked

[Liu Ying]

2012/6/11, Russell King - ARM Linux <linux@arm.linux.org.uk>:
> On Mon, Jun 11, 2012 at 06:46:14PM +0800, Liu Ying wrote:
>> 2012/6/11, Russell King - ARM Linux <linux@arm.linux.org.uk>:
>> > On Mon, Jun 11, 2012 at 09:06:48AM +0800, Liu Ying wrote:
>> >> This patch checks if framebuffer is unblanked before
>> >> we actually trigger panning in custom pan display
>> >> function.
>> >>
>> >> Signed-off-by: Liu Ying <Ying.Liu@freescale.com>
>> >> ---
>> >>  drivers/video/mx3fb.c |    5 +++++
>> >>  1 files changed, 5 insertions(+), 0 deletions(-)
>> >>
>> >> diff --git a/drivers/video/mx3fb.c b/drivers/video/mx3fb.c
>> >> index e3406ab..d53db60 100644
>> >> --- a/drivers/video/mx3fb.c
>> >> +++ b/drivers/video/mx3fb.c
>> >> @@ -1063,6 +1063,11 @@ static int mx3fb_pan_display(struct
>> >> fb_var_screeninfo *var,
>> >>  	dev_dbg(fbi->device, "%s [%c]\n", __func__,
>> >>  		list_empty(&mx3_fbi->idmac_channel->queue) ? '-' : '+');
>> >>
>> >> +	if (mx3_fbi->blank != FB_BLANK_UNBLANK) {
>> >> +		dev_dbg(fbi->device, "panning with fb blanked not supported\n");
>> >> +		return -EFAULT;
>> >> +	}
>> >
>> > Why is this an error, and why return -EFAULT?  What userspace access
>> > failed?
>> >
>> Hi, Russell,
>>
>> IMHO, panning with framebuffer blanked is meaningless, at least, it is
>> not a common usecase.
>
> Yes, but why should anything in userspace care whether the display is
> blanked or not?
>
> For example, we may pan the display when a user program produces output,
> and we're on the last line of the display.  We don't fail that just
> because the screen happens to be blank.  So I don't see why we should
> actively fail an explicit userspace pan request just because the console
> happened to be blanked.
>
> What I'm basically saying is that as far as _userspace_ is concerned,
> all the standard APIs should just work as normal whether the display is
> blanked or not.
>

Ok, understand your point. I am ok to drop this patch.

Thanks.

-- 
Best Regards,
Liu Ying