Acked-by: Fabien Dessenne <fabien.dessenne@xxxxxx> > -----Original Message----- > From: Mauro Carvalho Chehab [mailto:mchehab@xxxxxxxxxxxxxxx] > Sent: jeudi 11 juin 2015 12:37 > To: Fabien DESSENNE > Cc: Linux Media Mailing List; Mauro Carvalho Chehab > Subject: Re: [PATCH 2/2] [media] bdisp-debug: don't try to divide by s64 > > Hi Fabien, > > Em Thu, 11 Jun 2015 11:26:22 +0200 > Fabien DESSENNE <fabien.dessenne@xxxxxx> escreveu: > > > Hi Mauro, > > > > Please check my comments below. > > > > > -----Original Message----- > > > From: linux-media-owner@xxxxxxxxxxxxxxx [mailto:linux-media- > > > owner@xxxxxxxxxxxxxxx] On Behalf Of Mauro Carvalho Chehab > > > Sent: mercredi 10 juin 2015 22:59 > > > To: Linux Media Mailing List > > > Cc: Mauro Carvalho Chehab; Mauro Carvalho Chehab; Fabien DESSENNE > > > Subject: [PATCH 2/2] [media] bdisp-debug: don't try to divide by s64 > > > > > > There are several warnings there, on some architectures, related to > > > dividing a s32 by a s64 value: > > > > > > drivers/media/platform/sti/bdisp/bdisp-debug.c:594: warning: > > > comparison of distinct pointer types lacks a cast > > > drivers/media/platform/sti/bdisp/bdisp-debug.c:594: warning: right > > > shift count >= width of type > > > drivers/media/platform/sti/bdisp/bdisp-debug.c:594: warning: passing > > > argument 1 of '__div64_32' from incompatible pointer type > > > drivers/media/platform/sti/bdisp/bdisp-debug.c:595: warning: > > > comparison of distinct pointer types lacks a cast > > > drivers/media/platform/sti/bdisp/bdisp-debug.c:595: warning: right > > > shift count >= width of type > > > drivers/media/platform/sti/bdisp/bdisp-debug.c:595: warning: passing > > > argument 1 of '__div64_32' from incompatible pointer type CC [M] > > > drivers/media/tuners/mt2060.o > > > drivers/media/platform/sti/bdisp/bdisp-debug.c:596: warning: > > > comparison of distinct pointer types lacks a cast > > > drivers/media/platform/sti/bdisp/bdisp-debug.c:596: warning: right > > > shift count >= width of type > > > drivers/media/platform/sti/bdisp/bdisp-debug.c:596: warning: passing > > > argument 1 of '__div64_32' from incompatible pointer type > > > drivers/media/platform/sti/bdisp/bdisp-debug.c:597: warning: > > > comparison of distinct pointer types lacks a cast > > > drivers/media/platform/sti/bdisp/bdisp-debug.c:597: warning: right > > > shift count >= width of type > > > drivers/media/platform/sti/bdisp/bdisp-debug.c:597: warning: passing > > > argument 1 of '__div64_32' from incompatible pointer type > > > > > > That doesn't make much sense. What the driver is actually trying to > > > do is to divide one second by a value. So, check the range before > > > dividing. That warrants the right result and will remove the warnings on > non-64 bits archs. > > > > > > Also fixes this warning: > > > drivers/media/platform/sti/bdisp/bdisp-debug.c:588: warning: > > > comparison of distinct pointer types lacks a cast > > > > > > by using div64_s64() instead of calling do_div() directly. > > > > > > Signed-off-by: Mauro Carvalho Chehab <mchehab@xxxxxxxxxxxxxxx> > > > > > > diff --git a/drivers/media/platform/sti/bdisp/bdisp-debug.c > > > b/drivers/media/platform/sti/bdisp/bdisp-debug.c > > > index 7c3a632746ba..3f6f411aafdd 100644 > > > --- a/drivers/media/platform/sti/bdisp/bdisp-debug.c > > > +++ b/drivers/media/platform/sti/bdisp/bdisp-debug.c > > > @@ -572,6 +572,8 @@ static int bdisp_dbg_regs(struct seq_file *s, > > > void > > > *data) > > > return 0; > > > } > > > > > > +#define SECOND 1000000 > > > + > > > static int bdisp_dbg_perf(struct seq_file *s, void *data) { > > > struct bdisp_dev *bdisp = s->private; @@ -585,16 +587,27 @@ static > > > int bdisp_dbg_perf(struct seq_file *s, void > > > *data) > > > } > > > > > > avg_time_us = bdisp->dbg.tot_duration; > > > > When using div64_s64 the above line can be deleted, see my next > comment. > > > > > - do_div(avg_time_us, request->nb_req); > > > - > > > - avg_fps = 1000000; > > > - min_fps = 1000000; > > > - max_fps = 1000000; > > > - last_fps = 1000000; > > > - do_div(avg_fps, avg_time_us); > > > - do_div(min_fps, bdisp->dbg.min_duration); > > > - do_div(max_fps, bdisp->dbg.max_duration); > > > - do_div(last_fps, bdisp->dbg.last_duration); > > > + div64_s64(avg_time_us, request->nb_req); > > > > The operation result is returned by div64_s64(different from do_div that > updates the 1st parameter). > > The expected syntax is: > > avg_time_us = div64_s64(bdisp->dbg.tot_duration, request->nb_req); > > > > > + > > > + if (avg_time_us > SECOND) > > > + avg_fps = 0; > > > + else > > > + avg_fps = SECOND / (s32)avg_time_us; > > > + > > > + if (bdisp->dbg.min_duration > SECOND) > > > + min_fps = 0; > > > + else > > > + min_fps = SECOND / (s32)bdisp->dbg.min_duration); > > > > It probably builds better without the last unexpected parenthesis ;) > > Gah, a left-over... I did a first version using a different syntax. > > See version 2 below. > > > > + > > > + if (bdisp->dbg.max_duration > SECOND) > > > + max_fps = 0; > > > + else > > > + max_fps = SECOND / (s32)bdisp->dbg.max_duration; > > > + > > > + if (bdisp->dbg.last_duration > SECOND) > > > + last_fps = 0; > > > + else > > > + last_fps = SECOND / (s32)bdisp->dbg.last_duration; > > > > > > seq_printf(s, "HW processing (%d requests):\n", request->nb_req); > > > seq_printf(s, " Average: %5lld us (%3d fps)\n", > > > -- > > > 2.4.2 > > [PATCHv2] [media] bdisp-debug: don't try to divide by s64 > > There are several warnings there, on some architectures, related to dividing > a s32 by a s64 value: > > drivers/media/platform/sti/bdisp/bdisp-debug.c:594: warning: comparison > of distinct pointer types lacks a cast > drivers/media/platform/sti/bdisp/bdisp-debug.c:594: warning: right shift > count >= width of type > drivers/media/platform/sti/bdisp/bdisp-debug.c:594: warning: passing > argument 1 of '__div64_32' from incompatible pointer type > drivers/media/platform/sti/bdisp/bdisp-debug.c:595: warning: comparison > of distinct pointer types lacks a cast > drivers/media/platform/sti/bdisp/bdisp-debug.c:595: warning: right shift > count >= width of type > drivers/media/platform/sti/bdisp/bdisp-debug.c:595: warning: passing > argument 1 of '__div64_32' from incompatible pointer type CC [M] > drivers/media/tuners/mt2060.o > drivers/media/platform/sti/bdisp/bdisp-debug.c:596: warning: comparison > of distinct pointer types lacks a cast > drivers/media/platform/sti/bdisp/bdisp-debug.c:596: warning: right shift > count >= width of type > drivers/media/platform/sti/bdisp/bdisp-debug.c:596: warning: passing > argument 1 of '__div64_32' from incompatible pointer type > drivers/media/platform/sti/bdisp/bdisp-debug.c:597: warning: comparison > of distinct pointer types lacks a cast > drivers/media/platform/sti/bdisp/bdisp-debug.c:597: warning: right shift > count >= width of type > drivers/media/platform/sti/bdisp/bdisp-debug.c:597: warning: passing > argument 1 of '__div64_32' from incompatible pointer type > > That doesn't make much sense. What the driver is actually trying to do is to > divide one second by a value. So, check the range before dividing. That > warrants the right result and will remove the warnings on non-64 bits archs. > > Also fixes this warning: > drivers/media/platform/sti/bdisp/bdisp-debug.c:588: warning: comparison > of distinct pointer types lacks a cast > > by using div64_s64() instead of calling do_div() directly. > > Signed-off-by: Mauro Carvalho Chehab <mchehab@xxxxxxxxxxxxxxx> > > diff --git a/drivers/media/platform/sti/bdisp/bdisp-debug.c > b/drivers/media/platform/sti/bdisp/bdisp-debug.c > index 7c3a632746ba..18282a0f80c9 100644 > --- a/drivers/media/platform/sti/bdisp/bdisp-debug.c > +++ b/drivers/media/platform/sti/bdisp/bdisp-debug.c > @@ -572,6 +572,8 @@ static int bdisp_dbg_regs(struct seq_file *s, void > *data) > return 0; > } > > +#define SECOND 1000000 > + > static int bdisp_dbg_perf(struct seq_file *s, void *data) { > struct bdisp_dev *bdisp = s->private; > @@ -584,17 +586,26 @@ static int bdisp_dbg_perf(struct seq_file *s, void > *data) > return 0; > } > > - avg_time_us = bdisp->dbg.tot_duration; > - do_div(avg_time_us, request->nb_req); > - > - avg_fps = 1000000; > - min_fps = 1000000; > - max_fps = 1000000; > - last_fps = 1000000; > - do_div(avg_fps, avg_time_us); > - do_div(min_fps, bdisp->dbg.min_duration); > - do_div(max_fps, bdisp->dbg.max_duration); > - do_div(last_fps, bdisp->dbg.last_duration); > + avg_time_us = div64_s64(bdisp->dbg.tot_duration, request- > >nb_req); > + if (avg_time_us > SECOND) > + avg_fps = 0; > + else > + avg_fps = SECOND / (s32)avg_time_us; > + > + if (bdisp->dbg.min_duration > SECOND) > + min_fps = 0; > + else > + min_fps = SECOND / (s32)bdisp->dbg.min_duration; > + > + if (bdisp->dbg.max_duration > SECOND) > + max_fps = 0; > + else > + max_fps = SECOND / (s32)bdisp->dbg.max_duration; > + > + if (bdisp->dbg.last_duration > SECOND) > + last_fps = 0; > + else > + last_fps = SECOND / (s32)bdisp->dbg.last_duration; > > seq_printf(s, "HW processing (%d requests):\n", request->nb_req); > seq_printf(s, " Average: %5lld us (%3d fps)\n", -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
