Hi Tomi,
> -----Original Message-----
> From: Tomi Valkeinen [mailto:tomi.valkei...@nokia.com]
> Sent: Thursday, August 26, 2010 4:31 PM
> To: K, Mythri P
> Cc: linux-omap@vger.kernel.org; Semwal, Sumit
> Subject: Re: [PATCHv2] OMAP:DSS:Add support for Additional color modes
> supported by OMAP4
>
> On Thu, 2010-08-26 at 12:15 +0200, ext K, Mythri P wrote:
> > From: Sumit semwal <sumit.sem...@ti.com>
> >
> > Signed-off-by: Mythri P K <mythr...@ti.com>
> > ---
> > arch/arm/plat-omap/include/plat/display.h | 17 +++++++++-
> > drivers/video/omap2/dss/dispc.c | 53
> > ++++++++++++++++++++++-------
> > drivers/video/omap2/dss/overlay.c | 15 +++++---
> > 3 files changed, 66 insertions(+), 19 deletions(-)
>
> This doesn't apply. The patch seems to be based on some other kernel
> than mainline linux or my DSS2 tree.
>
This applies on top of set of patches sent by Archit.
> > diff --git a/arch/arm/plat-omap/include/plat/display.h
> > b/arch/arm/plat-omap/include/plat/display.h
> > index 7a6eedd..c8ff8f6 100644
> > --- a/arch/arm/plat-omap/include/plat/display.h
> > +++ b/arch/arm/plat-omap/include/plat/display.h
> > @@ -89,6 +89,12 @@ enum omap_color_mode {
> > OMAP_DSS_COLOR_ARGB32 = 1 << 11, /* ARGB32 */
> > OMAP_DSS_COLOR_RGBA32 = 1 << 12, /* RGBA32 */
> > OMAP_DSS_COLOR_RGBX32 = 1 << 13, /* RGBx32 */
> > + OMAP_DSS_COLOR_NV12 = 1 << 14, /* NV12 format: YUV 4:2:0 */
> > + OMAP_DSS_COLOR_RGBA12 = 1 << 15, /* RGBA12 - 4444 */
> > + OMAP_DSS_COLOR_XRGB12 = 1 << 16, /* xRGB12, 16-bit container */
>
> OMAP4's xRGB12-4444 is the same as OMAP3's RGB12
> (OMAP_DSS_COLOR_RGB12U), so there's no need to add XRGB12. Or is this
> supposed to be RGBx12-4444?
It is supposed to be RGB12x-4444 Let me correct this.
>
> > + OMAP_DSS_COLOR_ARGB16_1555 = 1 << 17, /* ARGB16-1555 */
> > + OMAP_DSS_COLOR_RGBX24_32_ALGN = 1 << 18, /* 32-msb aligned 24bit */
> > + OMAP_DSS_COLOR_XRGB15 = 1 << 19, /* xRGB15: 1555*/
>
> As these is kernel internal enum, I think it should be re-ordered to
> match OMAP4's color format order (the one in GFX/VID_ATTR:FORMAT
> register), to make it a bit more clear when comparing code and TRM.
>
I had arranged the enum OMAP_DSS_COLOR_VID_OMAP4 in TRM order,
I didn't want to change the existing order to make the diff clear , I can
change this.
> > OMAP_DSS_COLOR_GFX_OMAP2 =
> > OMAP_DSS_COLOR_CLUT1 | OMAP_DSS_COLOR_CLUT2 |
> > @@ -121,7 +127,16 @@ enum omap_color_mode {
> > OMAP_DSS_COLOR_UYVY | OMAP_DSS_COLOR_ARGB32 |
> > OMAP_DSS_COLOR_RGBA32 | OMAP_DSS_COLOR_RGBX32,
> >
> > - OMAP_DSS_COLOR_VID3_OMAP3 = OMAP_DSS_COLOR_VID2_OMAP3,
>
> There's no such line in the kernel.
Again this applies on top of patches sent by Archit and , his patch
https://patchwork.kernel.org/patch/112648/
Would add this line .I shall rebase it to your tree.
>
> > + OMAP_DSS_COLOR_VID_OMAP4 =
> > + OMAP_DSS_COLOR_RGB16 | OMAP_DSS_COLOR_RGB12U |
> > + OMAP_DSS_COLOR_YUV2 | OMAP_DSS_COLOR_ARGB16_1555 |
> > + OMAP_DSS_COLOR_RGBA32 | OMAP_DSS_COLOR_NV12 |
> > + OMAP_DSS_COLOR_RGBA12 | OMAP_DSS_COLOR_RGB24U |
> > + OMAP_DSS_COLOR_RGB24P | OMAP_DSS_COLOR_UYVY |
> > + OMAP_DSS_COLOR_ARGB16 | OMAP_DSS_COLOR_XRGB15 |
> > + OMAP_DSS_COLOR_ARGB32 | OMAP_DSS_COLOR_XRGB12 |
> > + OMAP_DSS_COLOR_RGBX24_32_ALGN,
>
> Does OMAP4 VID overlay support the same modes as OMAP3? If so, then this
> could be defined as OMAP_DSS_COLOR_VID2_OMAP3 | newmodes... to make it
> clear which modes are new. Or, if you want to state the modes
> explicitely, the order should be the same as in the existing OMAP3 enums
> to make it clear.
>
This was arranged in OMAP4 TRM order so, I can make an | of omap3 + new modes
supported by OMAP4.
> > };
> >
> > enum omap_lcd_display_type {
> > diff --git a/drivers/video/omap2/dss/dispc.c
> > b/drivers/video/omap2/dss/dispc.c
> > index d4a7e10..86702c5 100644
> > --- a/drivers/video/omap2/dss/dispc.c
> > +++ b/drivers/video/omap2/dss/dispc.c
> > @@ -1059,18 +1059,38 @@ static void _dispc_set_color_mode(enum omap_plane
> > plane,
> > enum omap_color_mode color_mode)
> > {
> > u32 m = 0;
> > -
> > switch (color_mode) {
> > - case OMAP_DSS_COLOR_CLUT1:
> > - m = 0x0; break;
> > - case OMAP_DSS_COLOR_CLUT2:
> > - m = 0x1; break;
> > - case OMAP_DSS_COLOR_CLUT4:
> > - m = 0x2; break;
> > - case OMAP_DSS_COLOR_CLUT8:
> > - m = 0x3; break;
> > + if ((!cpu_is_omap44xx()) || (OMAP_DSS_GFX == plane)) {
> > + case OMAP_DSS_COLOR_CLUT1:
> > + m = 0x0; break;
> > + case OMAP_DSS_COLOR_CLUT2:
> > + m = 0x1; break;
> > + case OMAP_DSS_COLOR_CLUT4:
> > + m = 0x2; break;
> > + case OMAP_DSS_COLOR_CLUT8:
> > + m = 0x3; break;
> > + case OMAP_DSS_COLOR_RGBX32:
> > + m = 0xe; break;
> > + } else {
> > + case OMAP_DSS_COLOR_NV12:
> > + m = 0x0; break;
> > + case OMAP_DSS_COLOR_RGBA12:
> > + m = 0x2; break;
> > + case OMAP_DSS_COLOR_XRGB12:
> > + m = 0x4; break;
> > + case OMAP_DSS_COLOR_ARGB16_1555:
> > + m = 0x7; break;
> > + case OMAP_DSS_COLOR_RGBX24_32_ALGN:
> > + m = 0xe; break;
> > + case OMAP_DSS_COLOR_XRGB15:
> > + m = 0xf; break;
> > + }
> > case OMAP_DSS_COLOR_RGB12U:
> > - m = 0x4; break;
> > + if ((!cpu_is_omap44xx()) || (OMAP_DSS_GFX == plane))
> > + m = 0x4;
> > + else
> > + m = 0x1;
> > + break;
>
> You have lots of ifs here. Isn't OMAP4's format register just extended
> from OMAP3, and so all the old modes work as such? And so there
> shouldn't be any need for ifs.
For OMAP4 video pipelines some of the values for additional color modes added
such as NV12,ARGB16_1555 are different from OMAP3 hence the if's.
>
> > case OMAP_DSS_COLOR_ARGB16:
> > m = 0x5; break;
> > case OMAP_DSS_COLOR_RGB16:
> > @@ -1087,8 +1107,6 @@ static void _dispc_set_color_mode(enum omap_plane
> > plane,
> > m = 0xc; break;
> > case OMAP_DSS_COLOR_RGBA32:
> > m = 0xd; break;
> > - case OMAP_DSS_COLOR_RGBX32:
> > - m = 0xe; break;
> > default:
> > BUG(); break;
> > }
> > @@ -1901,7 +1919,16 @@ static int _dispc_setup_plane(enum omap_plane plane,
> > case OMAP_DSS_COLOR_RGBA32:
> > if (cpu_is_omap24xx())
> > return -EINVAL;
> > - if (plane == OMAP_DSS_VIDEO1)
> > + if ((!cpu_is_omap44xx()) && (plane == OMAP_DSS_VIDEO1))
> > + return -EINVAL;
> > + break;
> > +
> > + case OMAP_DSS_COLOR_RGBA12:
> > + case OMAP_DSS_COLOR_XRGB12:
> > + case OMAP_DSS_COLOR_ARGB16_1555:
> > + case OMAP_DSS_COLOR_RGBX24_32_ALGN:
> > + case OMAP_DSS_COLOR_XRGB15:
> > + if (!cpu_is_omap44xx())
> > return -EINVAL;
> > break;
> >
> > diff --git a/drivers/video/omap2/dss/overlay.c
> > b/drivers/video/omap2/dss/overlay.c
> > index 0c189f8..4a2d908 100644
> > --- a/drivers/video/omap2/dss/overlay.c
> > +++ b/drivers/video/omap2/dss/overlay.c
> > @@ -588,7 +588,8 @@ void dss_init_overlays(struct platform_device *pdev)
> > case 0:
> > ovl->name = "gfx";
> > ovl->id = OMAP_DSS_GFX;
> > - ovl->supported_modes = cpu_is_omap34xx() ?
> > + ovl->supported_modes = (cpu_is_omap44xx() |
> > + cpu_is_omap34xx()) ?
> > OMAP_DSS_COLOR_GFX_OMAP3 :
> > OMAP_DSS_COLOR_GFX_OMAP2;
> > ovl->caps = OMAP_DSS_OVL_CAP_DISPC;
> > @@ -598,7 +599,9 @@ void dss_init_overlays(struct platform_device *pdev)
> > case 1:
> > ovl->name = "vid1";
> > ovl->id = OMAP_DSS_VIDEO1;
> > - ovl->supported_modes = cpu_is_omap34xx() ?
> > + ovl->supported_modes = cpu_is_omap44xx() ?
> > + OMAP_DSS_COLOR_VID_OMAP4 :
> > + cpu_is_omap34xx() ?
> > OMAP_DSS_COLOR_VID1_OMAP3 :
> > OMAP_DSS_COLOR_VID_OMAP2;
> > ovl->caps = OMAP_DSS_OVL_CAP_SCALE |
> > @@ -611,8 +614,10 @@ void dss_init_overlays(struct platform_device *pdev)
> > case 2:
> > ovl->name = "vid2";
> > ovl->id = OMAP_DSS_VIDEO2;
> > - ovl->supported_modes = cpu_is_omap34xx() ?
> > - OMAP_DSS_COLOR_VID2_OMAP3 :
> > + ovl->supported_modes = cpu_is_omap44xx() ?
> > + OMAP_DSS_COLOR_VID_OMAP4 :
> > + cpu_is_omap34xx() ?
> > + OMAP_DSS_COLOR_VID1_OMAP3 :
> > OMAP_DSS_COLOR_VID_OMAP2;
>
> This is getting a bit confusing. Perhaps a simple
> if (cpu_is_omap44xx())
> ovl->supported_modes = ...
> else if (cpu_is_omap34xx())
> ovl->supported_modes = ..
> ...
>
> would be more readable.
I can use nested "if else if" loop instead of nested " ? : " conditions.
>
> Tomi
>
Thanks and regards,
Mythri.
--
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
