Hi Tomi, > -----Original Message----- > From: Tomi Valkeinen [mailto:tomi.valkeinen@xxxxxxxxx] > Sent: Thursday, August 26, 2010 4:31 PM > To: K, Mythri P > Cc: linux-omap@xxxxxxxxxxxxxxx; 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.semwal@xxxxxx> > > > > Signed-off-by: Mythri P K <mythripk@xxxxxx> > > --- > > 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 majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
