Re: [V2 1/7] video: mmp: rb swap setting update for new LCD driver

Re: [V2 1/7] video: mmp: rb swap setting update for new LCD driver

[Daniel Drake]

On Mon, Jun 10, 2013 at 9:52 AM, Jett.Zhou <jtzhou@marvell.com> wrote:
> From: Guoqing Li <ligq@marvell.com>
>
> We could set rb swap in two modules: DMA controler and interface
> output after blending.
> This patch move the panel rbswap requirement setting in later module.

link_config originates from the platform's path config. link_config is
undocumented and this patch also changes its meaning.

Previously, bit 0 triggered rbswap, and this behaviour is relied upon
by arch/arm/mach-mmp/ttc_dkb.c

Now bits 27:24 of link_config are used to enable or disable rbswap,
and link_config bit 0 is ignored. According to the specs for the panel
path register, valid values for these new bits are 0 (no swap) and 1
(swap). ttc_dkb has not been updated in this patch series for this new
behaviour.

I don't understand why rbswap is set to 1 in fmt_to_reg for certain
RGB formats. The patch description suggests that the rb swapping can
either be set in fmt_to_reg context (to be written into DMA
controller) *or* in the interface output. If we are now relying on the
interface output control to do RB swapping when appropriate, why are
there still cases when we ask the DMA controller to do the same thing?

I also do not fully understand the requirement for RB swapping. I do
understand that it changes pixel format from RGB to BGR. What is the
point? I can imagine that some panels may require such a pixel format,
however in that case, this configuration should be part of the panel
configuration, not part of the path configuration as it currently is.

Daniel

Re: [V2 1/7] video: mmp: rb swap setting update for new LCD driver

[jett zhou]

2013/6/22 Daniel Drake <dsd@laptop.org>:
> On Mon, Jun 10, 2013 at 9:52 AM, Jett.Zhou <jtzhou@marvell.com> wrote:
>> From: Guoqing Li <ligq@marvell.com>
>>
>> We could set rb swap in two modules: DMA controler and interface
>> output after blending.
>> This patch move the panel rbswap requirement setting in later module.
>
> link_config originates from the platform's path config. link_config is
> undocumented and this patch also changes its meaning.
>
> Previously, bit 0 triggered rbswap, and this behaviour is relied upon
> by arch/arm/mach-mmp/ttc_dkb.c
>
> Now bits 27:24 of link_config are used to enable or disable rbswap,
> and link_config bit 0 is ignored. According to the specs for the panel
> path register, valid values for these new bits are 0 (no swap) and 1
> (swap). ttc_dkb has not been updated in this patch series for this new
> behaviour.
>
> I don't understand why rbswap is set to 1 in fmt_to_reg for certain
> RGB formats. The patch description suggests that the rb swapping can
> either be set in fmt_to_reg context (to be written into DMA
> controller) *or* in the interface output. If we are now relying on the
> interface output control to do RB swapping when appropriate, why are
> there still cases when we ask the DMA controller to do the same thing?
>
> I also do not fully understand the requirement for RB swapping. I do
> understand that it changes pixel format from RGB to BGR. What is the
> point? I can imagine that some panels may require such a pixel format,
> however in that case, this configuration should be part of the panel
> configuration, not part of the path configuration as it currently is.
>
> Daniel
 Hi Daniel
       Sorry that the comments of the patch is not so clear.
       As you might know,We can set two rbswap setting, one is based
on pix_fmt to set into DMA input part, the other is to configured in
the output interface part.
       This patch include below change and enhancement:
       1) The input format which support rbswap is based RGB format,
YUV series did not support now. So you can see the patch will change
the rbswap value based on specific RGB format,eg. RGB will set rbswap
= 1, BGR will not set it.
          This part setting is controlled in driver internally, will
not depend on platform configure(link_config) any more.
       2) The output part of rbswap is depend on link_config which is
defined in specific platfrom.
           It will setting bit27:24 of 0x01DC register, eg. DSI output
interface can be controlled by this.
           TTC_dkb does not support dsi, the link_config is no used anymore.
      I will add more this comments in the patch of V3.
Thanks


--

----------------------------------
Best Regards
Jett Zhou

Re: [V2 1/7] video: mmp: rb swap setting update for new LCD driver

[Daniel Drake]

On Mon, Jun 24, 2013 at 2:45 AM, jett zhou <jett.zhou@gmail.com> wrote:
>        Sorry that the comments of the patch is not so clear.
>        As you might know,We can set two rbswap setting, one is based on
> pix_fmt to set into DMA input part, the other is to configured in the output
> interface part.
>        This patch include below change and enhancement:
>        1) The input format which support rbswap is based RGB format, YUV
> series did not support now. So you can see the patch will change the rbswap
> value based on specific RGB format,eg. RGB will set rbswap = 1, BGR will not
> set it.

What if I'm working with a display that doesn't need or want RB
swapping? Lets say I am working with format PIXFMT_RGB565, and running
your patch. dmafetch_set_fmt() gets called, and fmt_to_reg() sets
rbswap to 1.
This means that dmafetch_set_fmt() writes a '1' into the appropriate
RB-swapping bit in the LCD_PN_CTRL0 register, and this triggers the
"DMA input" swapping that you mentioned. But I never asked for RB
swapping...

>           This part setting is controlled in driver internally, will not
> depend on platform configure(link_config) any more.

Your patch seems to depend very clearly on link_config.

>        2) The output part of rbswap is depend on link_config which is
> defined in specific platfrom.
>            It will setting bit27:24 of 0x01DC register, eg. DSI output
> interface can be controlled by this.

I don't think you should add more disorganised/undocumented (ab)use of
this magic link_config variable. You should create new, dedicated,
documented fields.

Your comment above suggests that this RB-swapping behaviour is
something that is imposed by the output device. In which case, this
should be a configuration parameter on the panel, not on the path
structure.

>            TTC_dkb does not support dsi, the link_config is no used anymore.

Then you should fix up ttc_dkb before submitting this patch.

Thanks
Daniel

Re: [V2 1/7] video: mmp: rb swap setting update for new LCD driver

[jett zhou]

2013/6/24 Daniel Drake <dsd@laptop.org>:
> On Mon, Jun 24, 2013 at 2:45 AM, jett zhou <jett.zhou@gmail.com> wrote:
>>        Sorry that the comments of the patch is not so clear.
>>        As you might know,We can set two rbswap setting, one is based on
>> pix_fmt to set into DMA input part, the other is to configured in the output
>> interface part.
>>        This patch include below change and enhancement:
>>        1) The input format which support rbswap is based RGB format, YUV
>> series did not support now. So you can see the patch will change the rbswap
>> value based on specific RGB format,eg. RGB will set rbswap = 1, BGR will not
>> set it.
>
> What if I'm working with a display that doesn't need or want RB
> swapping? Lets say I am working with format PIXFMT_RGB565, and running
> your patch. dmafetch_set_fmt() gets called, and fmt_to_reg() sets
> rbswap to 1.
> This means that dmafetch_set_fmt() writes a '1' into the appropriate
> RB-swapping bit in the LCD_PN_CTRL0 register, and this triggers the
> "DMA input" swapping that you mentioned. But I never asked for RB
> swapping...

Yes, if you configure it as PIXFMT_RGB565, it will set rbswap in "DMA
input" part.
So, for your case, you need to use PIXFMT_BGR565 instead of PIXFMT_RGB565.

>
>>           This part setting is controlled in driver internally, will not
>> depend on platform configure(link_config) any more.
>
> Your patch seems to depend very clearly on link_config.

Based on the patch, it does not depend on link_config for the "DMA
input" swapping, it depend on the configured pix_format.

>
>>        2) The output part of rbswap is depend on link_config which is
>> defined in specific platfrom.
>>            It will setting bit27:24 of 0x01DC register, eg. DSI output
>> interface can be controlled by this.
>
> I don't think you should add more disorganised/undocumented (ab)use of
> this magic link_config variable. You should create new, dedicated,
> documented fields.
Agree with you, create new,dedicated field for this usage is better, I
will amend the patch and send out for review again.:)
>
> Your comment above suggests that this RB-swapping behaviour is
> something that is imposed by the output device. In which case, this
> should be a configuration parameter on the panel, not on the path
> structure.
>
>>            TTC_dkb does not support dsi, the link_config is no used anymore.
>
> Then you should fix up ttc_dkb before submitting this patch.

After we add one new field for this output rbswap setting based on dsi
interface, it can be used by new stepping of mmp display controller,
ttc_dkb platform just leave and not touch it, it will be tranparent
for ttc_dkb, does not need to nothing for platform configuration for
ttc_dkb usage.
It means , ttc_dkb can only configure rbswap in "dma input" part, not
support rbswap in dsi interface part.
What do you think?

Thanks




--

----------------------------------
Best Regards
Jett Zhou

Re: [V2 1/7] video: mmp: rb swap setting update for new LCD driver

[Daniel Drake]

On Mon, Jun 24, 2013 at 8:23 PM, jett zhou <jett.zhou@gmail.com> wrote:
>> What if I'm working with a display that doesn't need or want RB
>> swapping? Lets say I am working with format PIXFMT_RGB565, and running
>> your patch. dmafetch_set_fmt() gets called, and fmt_to_reg() sets
>> rbswap to 1.
>> This means that dmafetch_set_fmt() writes a '1' into the appropriate
>> RB-swapping bit in the LCD_PN_CTRL0 register, and this triggers the
>> "DMA input" swapping that you mentioned. But I never asked for RB
>> swapping...
>
> Yes, if you configure it as PIXFMT_RGB565, it will set rbswap in "DMA
> input" part.
> So, for your case, you need to use PIXFMT_BGR565 instead of PIXFMT_RGB565.

So let me get this straight. I have a display that wants RGB565
format, no RB swapping. I don't do anything special in link_config to
affect any swapping.

After your patch, I must request BGR565 format in order to get RGB565?
That sounds backwards to me.

>> Your comment above suggests that this RB-swapping behaviour is
>> something that is imposed by the output device. In which case, this
>> should be a configuration parameter on the panel, not on the path
>> structure.
>>
>>>            TTC_dkb does not support dsi, the link_config is no used anymore.
>>
>> Then you should fix up ttc_dkb before submitting this patch.
>
> After we add one new field for this output rbswap setting based on dsi
> interface, it can be used by new stepping of mmp display controller,
> ttc_dkb platform just leave and not touch it, it will be tranparent
> for ttc_dkb, does not need to nothing for platform configuration for
> ttc_dkb usage.
> It means , ttc_dkb can only configure rbswap in "dma input" part, not
> support rbswap in dsi interface part.
> What do you think?

The point I am trying to make is that your patch is changing behaviour
for ttc_dkb, so you need to address that at the same time.

Right now ttc_dkb does this:

#define CFG_GRA_SWAPRB(x)      (x << 0) /* 1: rbswap enabled */
.link_config = CFG_DUMBMODE(0x2) | CFG_GRA_SWAPRB(0x1),

i.e. SWAPRB requested through bit 0 in link_config

And this is obeyed by the existing code in fmt_to_reg:

   u32 link_config = path_to_path_plat(overlay->path)->link_config;
   rbswap = link_config & 0x1;

Your patch removes the handling of link_config bit 0, without fixing
up its only user (even if that user was incorrect). That is not good
practice.

Another question: why is this change needed?
We can request rb swapping either in DMA input or in the output
interface. I can understand the driver maybe supporting one option or
the other. But after your patch, it seems like both are supported: RB
swapping could be enabled either through choice of input format, or
through configuration of output parameters.

Daniel

Re: [V2 1/7] video: mmp: rb swap setting update for new LCD driver

[jett zhou]

2013/6/25 Daniel Drake <dsd@laptop.org>:
> On Mon, Jun 24, 2013 at 8:23 PM, jett zhou <jett.zhou@gmail.com> wrote:
>>> What if I'm working with a display that doesn't need or want RB
>>> swapping? Lets say I am working with format PIXFMT_RGB565, and running
>>> your patch. dmafetch_set_fmt() gets called, and fmt_to_reg() sets
>>> rbswap to 1.
>>> This means that dmafetch_set_fmt() writes a '1' into the appropriate
>>> RB-swapping bit in the LCD_PN_CTRL0 register, and this triggers the
>>> "DMA input" swapping that you mentioned. But I never asked for RB
>>> swapping...
>>
>> Yes, if you configure it as PIXFMT_RGB565, it will set rbswap in "DMA
>> input" part.
>> So, for your case, you need to use PIXFMT_BGR565 instead of PIXFMT_RGB565.
>
> So let me get this straight. I have a display that wants RGB565
> format, no RB swapping. I don't do anything special in link_config to
> affect any swapping.
>
> After your patch, I must request BGR565 format in order to get RGB565?
> That sounds backwards to me.

HI Daniel
     My above explanation may be not so precise and correct, actually
you can RGB565 directly and does not need BGR565, mmp display
controller handle it accordingly in driver.
     Take RGB565 and BGR565 as example:
          RGB565 indicates the source data in memory is that Red is
[15~11] , Green is [10~5], Blue is [4:0], Red is MSB, Blue is LSB, but
for the display dma input default setting(rbswap = 0), it only support
Blue is [15~11] , Green is [10~5], Red is [4:0], Red is LSB, Blue is
MSB, so for this format(RGB565), display controller need to set rbswap
= 1 and it can support the MSB/LSB correctly.
          In conclusion, no matter you use RGB565 or BGR565, mmp
display driver did the rbswap setiing based on the format and what is
MSB/LSB, so platform does not need handle it by link_config based on
this patch.
Thanks

>
>>> Your comment above suggests that this RB-swapping behaviour is
>>> something that is imposed by the output device. In which case, this
>>> should be a configuration parameter on the panel, not on the path
>>> structure.
>>>
>>>>            TTC_dkb does not support dsi, the link_config is no used anymore.
>>>
>>> Then you should fix up ttc_dkb before submitting this patch.
>>
>> After we add one new field for this output rbswap setting based on dsi
>> interface, it can be used by new stepping of mmp display controller,
>> ttc_dkb platform just leave and not touch it, it will be tranparent
>> for ttc_dkb, does not need to nothing for platform configuration for
>> ttc_dkb usage.
>> It means , ttc_dkb can only configure rbswap in "dma input" part, not
>> support rbswap in dsi interface part.
>> What do you think?
>
> The point I am trying to make is that your patch is changing behaviour
> for ttc_dkb, so you need to address that at the same time.
>
> Right now ttc_dkb does this:
>
> #define CFG_GRA_SWAPRB(x)      (x << 0) /* 1: rbswap enabled */
> .link_config = CFG_DUMBMODE(0x2) | CFG_GRA_SWAPRB(0x1),
>
> i.e. SWAPRB requested through bit 0 in link_config
>
> And this is obeyed by the existing code in fmt_to_reg:
>
>    u32 link_config = path_to_path_plat(overlay->path)->link_config;
>    rbswap = link_config & 0x1;
>
> Your patch removes the handling of link_config bit 0, without fixing
> up its only user (even if that user was incorrect). That is not good
> practice.
>
> Another question: why is this change needed?
> We can request rb swapping either in DMA input or in the output
> interface. I can understand the driver maybe supporting one option or
> the other. But after your patch, it seems like both are supported: RB
> swapping could be enabled either through choice of input format, or
> through configuration of output parameters.
For TTC dkb,  you are right, I need to fix the rbswap in the platform
part, I will add one fix patch for ttc_dkb based on new rbswap patch.
For output dsi interface, it has this feature to do rbswap again if it
needs specifc byte sequence of RGB byte of DSI panel.
eg. If display content is set RGB565 in memory and DMA input part set
rbswap in driver to support Red as MSB , Blue LSB, but dsi panel only
support Red as LSB, Blue as MSB, then it can use this feature.
If there is no this requirement of panel, this dsi output part is not needed.

Thanks
>




--

----------------------------------
Best Regards
Jett Zhou