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.
Signed-off-by: Liu Ying <[email protected]>
---
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 */
+
+ 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)
+{
+ 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;
+
/* 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;
+
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 */
-
y_bottom = var->yoffset;
if (!(var->vmode & FB_VMODE_YWRAP))
--
1.7.1
Add i.mx SoC maintainer Sacha and ARM maillist, and update Guennadi's email address.
Liu Ying
-----Original Message-----
From: Liu Ying-B17645
Sent: Wednesday, May 30, 2012 12:27 PM
To: [email protected]
Cc: [email protected]; [email protected]; [email protected]; Liu Ying-B17645
Subject: [PATCH 1/1] mx3fb: support pan display with fb_set_var
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.
Signed-off-by: Liu Ying <[email protected]>
---
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 */
+
+ 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) {
+ 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;
+
/* 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;
+
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 */
-
y_bottom = var->yoffset;
if (!(var->vmode & FB_VMODE_YWRAP))
--
1.7.1
On Wed, 30 May 2012, 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 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.
>
> Signed-off-by: Liu Ying <[email protected]>
> ---
> 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.
> +
> + 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()?
> +{
> + 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.
> /* 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().
> 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.
> 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/
2012/6/6, Guennadi Liakhovetski <[email protected]>:
> On Wed, 30 May 2012, 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 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.
>>
>> Signed-off-by: Liu Ying <[email protected]>
>> ---
>> 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.
>
>> +
>> + 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.
>
>> +{
>> + 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.
>
>> /* 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.
>
>> 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.
>
>> 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 [email protected]
> 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