Re: [PATCH 1/2] OMAPDSS: DISPC: Update Fir Coefficients

[Tomi Valkeinen <tomi.valkeinen@ti.com>]

[-- Attachment #1: Type: text/plain, Size: 4198 bytes --]

On Fri, 2011-12-16 at 10:26 +0530, Mahapatra, Chandrabhanu wrote:
> On Thu, Dec 15, 2011 at 2:25 PM, Tomi Valkeinen <tomi.valkeinen@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


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]