RE: [PATCHv2] OMAP:DSS:Add support for Additional color modes supported by OMAP4

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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


[Index of Archives]     [Linux Arm (vger)]     [ARM Kernel]     [ARM MSM]     [Linux Tegra]     [Linux WPAN Networking]     [Linux Wireless Networking]     [Maemo Users]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Trails]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux