2012/6/6, Guennadi Liakhovetski [off-list ref]:

On Wed, 30 May 2012, Liu Ying wrote:

quoted

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 display flash
if fb_set_var() is called to do panning frequently. The custom
fb_pan_display() method checks if the current xoffset and
yoffset are different from previous ones before doing actual
panning, which prevents the panning from happening within the
fb_set_var() context. This patch checks new var info 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. Meanwhile, this patch removes
the check in custom fb_pan_display() method mentioned before to
have the panning work within fb_set_var() context. It doesn't
hurt to do panning again if there is no update for xoffset and
yoffset.

You are really addressing 2 separate problems here: (1) panning cannot be
set using FBIOPUT_VSCREENINFO and (2) screen flashes every time
fb_set_var() is called, even if only panning is required. The reason for
the first one is, that in fb_set_var() info->var is already updated from
the new *var when fb_pan_display() is called. So, as you correctly
identified, the condition

	if (fbi->var.xoffset = var->xoffset &&
	    fbi->var.yoffset = var->yoffset)
		return 0;	/* No change, do nothing */

is trivially met and no panning takes place. Instead, you can use your
idea to cache var_info locally and check against that one to see, whether
offsets have changed, instead of removing that check completely.

To solve the second problem you can use your check against the cached copy
of var_info. See below for more details.

Thanks for your review. I think your suggestion is good and I have
sent you an updated series of patches for your review. Please find
more detail feedback inline.

quoted

Signed-off-by: Liu Ying <redacted>
---
 drivers/video/mx3fb.c |   31 +++++++++++++++++++++++++++----
 1 files changed, 27 insertions(+), 4 deletions(-)

diff --git a/drivers/video/mx3fb.c b/drivers/video/mx3fb.c
index e3406ab..238b9aa 100644
--- a/drivers/video/mx3fb.c
+++ b/drivers/video/mx3fb.c

@@ -269,6 +269,8 @@ struct mx3fb_info {
 	struct scatterlist		sg[2];

 	u32				sync;	/* preserve var->sync flags */

An incremental patch could then also remove the above .sync member and
switch to using .cur_var.sync instead.

Ok. I have addressed this in an updated patch.

quoted

+
+	struct fb_var_screeninfo	cur_var; /* current var info */
 };

 static void mx3fb_dma_done(void *);

@@ -721,6 +723,26 @@ static void mx3fb_dma_done(void *arg)
 	complete(&mx3_fbi->flip_cmpl);
 }

+static bool mx3fb_need_not_to_set_par(struct fb_info *fbi)

How about inverting logic and calling the function
mx3fb_must_update_par()?

Ok. I use mx3fb_must_set_par() in an updated patch.

quoted

+{
+	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 false;
+
+	/*
+	 * 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;

@@ -732,6 +754,9 @@ static int __set_par(struct fb_info *fbi, bool lock)
 	struct idmac_video_param *video = &ichan->params.video;
 	struct scatterlist *sg = mx3_fbi->sg;

+	if (mx3fb_need_not_to_set_par(fbi))
+		return 0;
+

__set_par() is called from 2 locations: from mx3fb_set_par() and
init_fb_chan(), called from probing. You don't need to perform the above
check in init_fb_chan() - there you always have to configure. Maybe better
put it in mx3fb_set_par() just before calling __set_par() like

	ret = mx3fb_must_set_par() ? __set_par() : 0;

As mentioned above, this solves problem #2 and should go into patch #2.

Ok. I have implemented your suggestion in an updated patch and
splitted this single patch into 2 separate ones. One is to support pan
display with fb_set_var(), the other is to fix screen flash issue when
panning with fb_set_var() frequently.

quoted

 	/* Total cleanup */
 	if (mx3_fbi->txd)
 		sdc_disable_channel(mx3_fbi);

@@ -808,6 +833,8 @@ static int __set_par(struct fb_info *fbi, bool lock)
 	if (mx3_fbi->blank = FB_BLANK_UNBLANK)
 		sdc_enable_channel(mx3_fbi);

+	mx3_fbi->cur_var = fbi->var;
+

Yes, but preserve xoffset and yoffset - you don't apply them yet in
__set_par().

Yes. I have perserved xoffset and yoffset in case fb is blanked when
calling __set_par() in an updated patch. I found that in case fb is
unblanked when calling __set_par(), fb smem_start address is set to
the controller, so I set mx3_fbi->cur_var.xoffset and
mx3_fbi->cur_var.yoffset to zero in __set_par() in this case. Please
fix me if I misunderstood anything.

quoted

 	return 0;
 }

@@ -1068,10 +1095,6 @@ static int mx3fb_pan_display(struct

fb_var_screeninfo *var,
 		return -EINVAL;
 	}

-	if (fbi->var.xoffset = var->xoffset &&
-	    fbi->var.yoffset = var->yoffset)
-		return 0;	/* No change, do nothing */
-

I think, it would be better not to remove these completely, but check
against cached .cur_var, and then update those values too.

Ok. I have addressed this in an updated patch.

quoted

 	y_bottom = var->yoffset;

 	if (!(var->vmode & FB_VMODE_YWRAP))
--
1.7.1

Makes sense? Or have I misunderstood something?

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

-- 
Best Regards,
Liu Ying