Re: [PATCH 1/2] OMAPDSS: DISPC: Update Fir Coefficients
On Fri, 2011-12-16 at 10:26 +0530, Mahapatra, Chandrabhanu wrote:
On Thu, Dec 15, 2011 at 2:25 PM, Tomi Valkeinen tomi.valkei...@ti.com wrote:
On Wed, 2011-12-14 at 10:21 +0530, Chandrabhanu Mahapatra wrote:
+const struct dispc_coef *dispc_ovl_get_scale_coef(int inc, int five_taps)
+{
+ int i;
+ static const struct {
+ int Mmin;
+ int Mmax;
+ const struct dispc_coef *coef_3;
+ const struct dispc_coef *coef_5;
+ } coefs[] = {
+ { 26, 32, coef3_M32, coef5_M32 },
+ { 22, 26, coef3_M26, coef5_M26 },
+ { 19, 22, coef3_M22, coef5_M22 },
+ { 16, 19, coef3_M19, coef5_M19 },
+ { 14, 16, coef3_M16, coef5_M16 },
+ { 13, 14, coef3_M14, coef5_M14 },
+ { 12, 13, coef3_M13, coef5_M13 },
+ { 11, 12, coef3_M12, coef5_M12 },
+ { 10, 11, coef3_M11, coef5_M11 },
+ { 9, 10, coef3_M10, coef5_M10 },
+ { 8, 9, coef3_M9, coef5_M9 },
+ { 3, 8, coef3_M8, coef5_M8 },
+ /*
+ * When upscaling more than two times, blockiness and
outlines
+ * around the image are observed when M8 tables are used.
M11,
+ * M16 and M19 tables are used to prevent this.
+ */
+ { 2, 3, coef3_M11, coef5_M11 },
+ { 1, 2, coef3_M16, coef5_M16 },
+ };
+
+ inc /= 128;
+ for (i = 0; i ARRAY_LEN(coefs); ++i)
+ if (inc coefs[i].Mmin inc = coefs[i].Mmax)
+ return five_taps ? coefs[i].coef_5 : coefs[i].coef_3;
+ if (inc == 1)
+ return five_taps ? coef3_M19 : coef5_M19;
+ return NULL;
+}
Why don't you handle the inc == 1 case the same as others? Just have an
entry in the table for Mmin=0, Mmax = 1.
For inc=1 i.e. M=1 , scaling ratio is maximum as L/M=8. DISPC scaler
doesnot support upscaling more than 8 itmes. Having an (Mmin,Mmax] of
(0,1] will allow such cases.
I don't think I understand. A table entry for 0,1 would match exactly
one inc value, which is 1. Which is the same as you do with the separate
if statement now.
Also, I think it's a bit confusing that Mmin is exclusive and Mmax is
inclusive in the comparison. It makes the table a bit hard to read, when
looking at which entry is used for which inc. I'd recommend using
inclusive comparison for both.
Tomi
Having both inclusive will allow us to delete the extra comparison for
inc==1 but in my opinion having Mmin exclusive and Mmax inclusive
actually gives an clear idea of comparison. The tables mostly go by
the Mmax value.
For example, for inc=26 coef3/5_M26 table is selected, for inc=22
coef3/5_M22 is selected etc.
If we have both Mmin and Mmax as inclusive above case becomes slightly
incoherent. Say for M=26 instead of coef3/5_M26 which seems more
obvious choice coef3/5_M32 is selected.
I don't understand this either... If you now have:
{ 26, 32, coef3_M32, coef5_M32 },
{ 22, 26, coef3_M26, coef5_M26 },
It would be changed to
{ 27, 32, coef3_M32, coef5_M32 },
{ 23, 26, coef3_M26, coef5_M26 },
and it would match the same inc values as before after changing the Mmin
comparison to =.
For both inclusive cases to work and avoid confusion and delete extra
comparison for inc==1 , I have to reverse the order of table entries
in coef table. But for that I will have to put the When upscaling
more than two times, blockiness and outlines comment at the beginning
of the table and then start with { 1, 2, coef3_M16, coef5_M16 }.
This will create even more confusion.
The ranges for the table elements are exclusive. The order doesn't
matter because one inc value can only match one table entry. So I have
to say I don't understand this comment either =). Am I missing
something?
Tomi
signature.asc
Description: This is a digitally signed message part
Re: [PATCH 1/2] OMAPDSS: DISPC: Update Fir Coefficients
On Fri, Dec 16, 2011 at 1:45 PM, Tomi Valkeinen tomi.valkei...@ti.com wrote:
On Fri, 2011-12-16 at 10:26 +0530, Mahapatra, Chandrabhanu wrote:
On Thu, Dec 15, 2011 at 2:25 PM, Tomi Valkeinen tomi.valkei...@ti.com
wrote:
On Wed, 2011-12-14 at 10:21 +0530, Chandrabhanu Mahapatra wrote:
+const struct dispc_coef *dispc_ovl_get_scale_coef(int inc, int five_taps)
+{
+ int i;
+ static const struct {
+ int Mmin;
+ int Mmax;
+ const struct dispc_coef *coef_3;
+ const struct dispc_coef *coef_5;
+ } coefs[] = {
+ { 26, 32, coef3_M32, coef5_M32 },
+ { 22, 26, coef3_M26, coef5_M26 },
+ { 19, 22, coef3_M22, coef5_M22 },
+ { 16, 19, coef3_M19, coef5_M19 },
+ { 14, 16, coef3_M16, coef5_M16 },
+ { 13, 14, coef3_M14, coef5_M14 },
+ { 12, 13, coef3_M13, coef5_M13 },
+ { 11, 12, coef3_M12, coef5_M12 },
+ { 10, 11, coef3_M11, coef5_M11 },
+ { 9, 10, coef3_M10, coef5_M10 },
+ { 8, 9, coef3_M9, coef5_M9 },
+ { 3, 8, coef3_M8, coef5_M8 },
+ /*
+ * When upscaling more than two times, blockiness and
outlines
+ * around the image are observed when M8 tables are used.
M11,
+ * M16 and M19 tables are used to prevent this.
+ */
+ { 2, 3, coef3_M11, coef5_M11 },
+ { 1, 2, coef3_M16, coef5_M16 },
+ };
+
+ inc /= 128;
+ for (i = 0; i ARRAY_LEN(coefs); ++i)
+ if (inc coefs[i].Mmin inc = coefs[i].Mmax)
+ return five_taps ? coefs[i].coef_5 :
coefs[i].coef_3;
+ if (inc == 1)
+ return five_taps ? coef3_M19 : coef5_M19;
+ return NULL;
+}
Why don't you handle the inc == 1 case the same as others? Just have an
entry in the table for Mmin=0, Mmax = 1.
For inc=1 i.e. M=1 , scaling ratio is maximum as L/M=8. DISPC scaler
doesnot support upscaling more than 8 itmes. Having an (Mmin,Mmax] of
(0,1] will allow such cases.
I don't think I understand. A table entry for 0,1 would match exactly
one inc value, which is 1. Which is the same as you do with the separate
if statement now.
yes, you are right.
Also, I think it's a bit confusing that Mmin is exclusive and Mmax is
inclusive in the comparison. It makes the table a bit hard to read, when
looking at which entry is used for which inc. I'd recommend using
inclusive comparison for both.
Tomi
Having both inclusive will allow us to delete the extra comparison for
inc==1 but in my opinion having Mmin exclusive and Mmax inclusive
actually gives an clear idea of comparison. The tables mostly go by
the Mmax value.
For example, for inc=26 coef3/5_M26 table is selected, for inc=22
coef3/5_M22 is selected etc.
If we have both Mmin and Mmax as inclusive above case becomes slightly
incoherent. Say for M=26 instead of coef3/5_M26 which seems more
obvious choice coef3/5_M32 is selected.
I don't understand this either... If you now have:
{ 26, 32, coef3_M32, coef5_M32 },
{ 22, 26, coef3_M26, coef5_M26 },
It would be changed to
{ 27, 32, coef3_M32, coef5_M32 },
{ 23, 26, coef3_M26, coef5_M26 },
and it would match the same inc values as before after changing the Mmin
comparison to =.
yes, I had mistakenly overlooked it.
For both inclusive cases to work and avoid confusion and delete extra
comparison for inc==1 , I have to reverse the order of table entries
in coef table. But for that I will have to put the When upscaling
more than two times, blockiness and outlines comment at the beginning
of the table and then start with { 1, 2, coef3_M16, coef5_M16 }.
This will create even more confusion.
The ranges for the table elements are exclusive. The order doesn't
matter because one inc value can only match one table entry. So I have
to say I don't understand this comment either =). Am I missing
something?
Tomi
I mistakenly thought inc to be float which created all this confusion.
--
Chandrabhanu Mahapatra
Texas Instruments India Pvt. Ltd.
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] OMAPDSS: DISPC: Update Fir Coefficients
On Wed, 2011-12-14 at 10:21 +0530, Chandrabhanu Mahapatra wrote:
+const struct dispc_coef *dispc_ovl_get_scale_coef(int inc, int five_taps)
+{
+ int i;
+ static const struct {
+ int Mmin;
+ int Mmax;
+ const struct dispc_coef *coef_3;
+ const struct dispc_coef *coef_5;
+ } coefs[] = {
+ { 26, 32, coef3_M32, coef5_M32 },
+ { 22, 26, coef3_M26, coef5_M26 },
+ { 19, 22, coef3_M22, coef5_M22 },
+ { 16, 19, coef3_M19, coef5_M19 },
+ { 14, 16, coef3_M16, coef5_M16 },
+ { 13, 14, coef3_M14, coef5_M14 },
+ { 12, 13, coef3_M13, coef5_M13 },
+ { 11, 12, coef3_M12, coef5_M12 },
+ { 10, 11, coef3_M11, coef5_M11 },
+ { 9, 10, coef3_M10, coef5_M10 },
+ { 8, 9, coef3_M9, coef5_M9 },
+ { 3, 8, coef3_M8, coef5_M8 },
+ /*
+ * When upscaling more than two times, blockiness and outlines
+ * around the image are observed when M8 tables are used. M11,
+ * M16 and M19 tables are used to prevent this.
+ */
+ { 2, 3, coef3_M11, coef5_M11 },
+ { 1, 2, coef3_M16, coef5_M16 },
+ };
+
+ inc /= 128;
+ for (i = 0; i ARRAY_LEN(coefs); ++i)
+ if (inc coefs[i].Mmin inc = coefs[i].Mmax)
+ return five_taps ? coefs[i].coef_5 : coefs[i].coef_3;
+ if (inc == 1)
+ return five_taps ? coef3_M19 : coef5_M19;
+ return NULL;
+}
Why don't you handle the inc == 1 case the same as others? Just have an
entry in the table for Mmin=0, Mmax = 1.
Also, I think it's a bit confusing that Mmin is exclusive and Mmax is
inclusive in the comparison. It makes the table a bit hard to read, when
looking at which entry is used for which inc. I'd recommend using
inclusive comparison for both.
Tomi
signature.asc
Description: This is a digitally signed message part
Re: [PATCH 1/2] OMAPDSS: DISPC: Update Fir Coefficients
On Thu, Dec 15, 2011 at 2:25 PM, Tomi Valkeinen tomi.valkei...@ti.com wrote:
On Wed, 2011-12-14 at 10:21 +0530, Chandrabhanu Mahapatra wrote:
+const struct dispc_coef *dispc_ovl_get_scale_coef(int inc, int five_taps)
+{
+ int i;
+ static const struct {
+ int Mmin;
+ int Mmax;
+ const struct dispc_coef *coef_3;
+ const struct dispc_coef *coef_5;
+ } coefs[] = {
+ { 26, 32, coef3_M32, coef5_M32 },
+ { 22, 26, coef3_M26, coef5_M26 },
+ { 19, 22, coef3_M22, coef5_M22 },
+ { 16, 19, coef3_M19, coef5_M19 },
+ { 14, 16, coef3_M16, coef5_M16 },
+ { 13, 14, coef3_M14, coef5_M14 },
+ { 12, 13, coef3_M13, coef5_M13 },
+ { 11, 12, coef3_M12, coef5_M12 },
+ { 10, 11, coef3_M11, coef5_M11 },
+ { 9, 10, coef3_M10, coef5_M10 },
+ { 8, 9, coef3_M9, coef5_M9 },
+ { 3, 8, coef3_M8, coef5_M8 },
+ /*
+ * When upscaling more than two times, blockiness and outlines
+ * around the image are observed when M8 tables are used. M11,
+ * M16 and M19 tables are used to prevent this.
+ */
+ { 2, 3, coef3_M11, coef5_M11 },
+ { 1, 2, coef3_M16, coef5_M16 },
+ };
+
+ inc /= 128;
+ for (i = 0; i ARRAY_LEN(coefs); ++i)
+ if (inc coefs[i].Mmin inc = coefs[i].Mmax)
+ return five_taps ? coefs[i].coef_5 : coefs[i].coef_3;
+ if (inc == 1)
+ return five_taps ? coef3_M19 : coef5_M19;
+ return NULL;
+}
Why don't you handle the inc == 1 case the same as others? Just have an
entry in the table for Mmin=0, Mmax = 1.
For inc=1 i.e. M=1 , scaling ratio is maximum as L/M=8. DISPC scaler
doesnot support upscaling more than 8 itmes. Having an (Mmin,Mmax] of
(0,1] will allow such cases.
Also, I think it's a bit confusing that Mmin is exclusive and Mmax is
inclusive in the comparison. It makes the table a bit hard to read, when
looking at which entry is used for which inc. I'd recommend using
inclusive comparison for both.
Tomi
Having both inclusive will allow us to delete the extra comparison for
inc==1 but in my opinion having Mmin exclusive and Mmax inclusive
actually gives an clear idea of comparison. The tables mostly go by
the Mmax value.
For example, for inc=26 coef3/5_M26 table is selected, for inc=22
coef3/5_M22 is selected etc.
If we have both Mmin and Mmax as inclusive above case becomes slightly
incoherent. Say for M=26 instead of coef3/5_M26 which seems more
obvious choice coef3/5_M32 is selected.
For both inclusive cases to work and avoid confusion and delete extra
comparison for inc==1 , I have to reverse the order of table entries
in coef table. But for that I will have to put the When upscaling
more than two times, blockiness and outlines comment at the beginning
of the table and then start with { 1, 2, coef3_M16, coef5_M16 }.
This will create even more confusion.
--
Chandrabhanu Mahapatra
Texas Instruments India Pvt. Ltd.
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
