Hi,
On Fri, Feb 12, 2016 at 5:04 PM, Inki Dae <inki.dae at samsung.com> wrote:
> Hi Chanho,
>
> 2016ë
02ì 12ì¼ 16:39ì Chanho Park ì´(ê°) ì´ ê¸:
>> Hi,
>>
>> On Fri, Feb 12, 2016 at 1:56 PM, Inki Dae <inki.dae at samsung.com> wrote:
>>> Hi Chanho,
>>>
>>> There was a missing one so below is my review again.
>>>
>>> 2016ë
02ì 11ì¼ 22:59ì Chanho Park ì´(ê°) ì´ ê¸:
>>>> Hi Inki,
>>>>
>>>> On Thu, Feb 11, 2016 at 7:37 PM, Inki Dae <inki.dae at samsung.com> wrote:
>>>>> Hi Chanho,
>>>>>
>>>>> 2016ë
01ì 30ì¼ 22:58ì Chanho Park ì´(ê°) ì´ ê¸:
>>>>>> From: Chanho Park <chanho61.park at samsung.com>
>>>>>>
>>>>>> This patch adds a mic_bypass option to bypass the mic
>>>>>> from display out path. The mic(Mobile image compressor) compresses
>>>>>> RGB data from fimd and send the compressed data to the mipi dsi.
>>>>>> The bypass option can be founded from system register and the bit
>>>>>> of the option is 11.
>>>>>>
>>>>>> Cc: Inki Dae <inki.dae at samsung.com>
>>>>>> Cc: Joonyoung Shim <jy0922.shim at samsung.com>
>>>>>> Cc: Seung-Woo Kim <sw0312.kim at samsung.com>
>>>>>> Signed-off-by: Chanho Park <chanho61.park at samsung.com>
>>>>>> ---
>>>>>> .../devicetree/bindings/display/exynos/samsung-fimd.txt | 2 ++
>>>>>> drivers/gpu/drm/exynos/exynos_drm_fimd.c | 14
>>>>>> ++++++++++++++
>>>>>> 2 files changed, 16 insertions(+)
>>>>>>
>>>>>> diff --git
>>>>>> a/Documentation/devicetree/bindings/display/exynos/samsung-fimd.txt
>>>>>> b/Documentation/devicetree/bindings/display/exynos/samsung-fimd.txt
>>>>>> index 27c3ce0..7f90c4a 100644
>>>>>> --- a/Documentation/devicetree/bindings/display/exynos/samsung-fimd.txt
>>>>>> +++ b/Documentation/devicetree/bindings/display/exynos/samsung-fimd.txt
>>>>>> @@ -45,6 +45,8 @@ Optional Properties:
>>>>>> Can be used in case timings cannot be provided otherwise
>>>>>> or to override timings provided by the panel.
>>>>>> - samsung,sysreg: handle to syscon used to control the system registers
>>>>>> +- samsung,mic-bypass: bypass mic(mobile image compressor) from display
>>>>>> path.
>>>
>>> mic-bypass is not really common property for fimd family so it's not
>>> correct for fimd device node has mic-bypass property.
>>
>> According to user manual of exynos, the option has been introduced
>> since exynos5420.
>
> That is why samsung-fimd.txt is not correct place. samsung-fimd.txt describes
> hardware information for s3c24xx, s3c64xx also, which don't have mic ip.
Yes. I just described the option's limitation like below.
>
>> The exynos5420 and exynos5422(5800) have a fimd controller not decon
>> display controller.
>>
>>>
>>>>>> + This option is only available since exynos5420.
I described the limitation in here.
>>>>>> - i80-if-timings: timing configuration for lcd i80 interface support.
>>>>>> - cs-setup: clock cycles for the active period of address signal is
>>>>>> enabled
>>>>>> until chip select is enabled.
>>>>>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
>>>>>> b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
>>>>>> index 70194d0..4fb2952 100644
>>>>>> --- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
>>>>>> +++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
>>>>>> @@ -94,6 +94,7 @@ struct fimd_driver_data {
>>>>>> unsigned int lcdblk_offset;
>>>>>> unsigned int lcdblk_vt_shift;
>>>>>> unsigned int lcdblk_bypass_shift;
>>>>>> + unsigned int lcdblk_mic_bypass_shift;
>>>>>>
>>>>>> unsigned int has_shadowcon:1;
>>>>>> unsigned int has_clksel:1;
>>>>>> @@ -140,6 +141,7 @@ static struct fimd_driver_data
>>>>>> exynos5_fimd_driver_data = {
>>>>>> .lcdblk_offset = 0x214,
>>>>>> .lcdblk_vt_shift = 24,
>>>>>> .lcdblk_bypass_shift = 15,
>>>>>> + .lcdblk_mic_bypass_shift = 11,
>>>>>> .has_shadowcon = 1,
>>>>>> .has_vidoutcon = 1,
>>>>>> .has_vtsel = 1,
>>>>>> @@ -162,6 +164,7 @@ struct fimd_context {
>>>>>> u32 i80ifcon;
>>>>>> bool i80_if;
>>>>>> bool suspended;
>>>>>> + bool mic_bypass;
>>>>>> int pipe;
>>>>>> wait_queue_head_t wait_vsync_queue;
>>>>>> atomic_t wait_vsync_event;
>>>>>> @@ -461,6 +464,14 @@ static void fimd_commit(struct exynos_drm_crtc
>>>>>> *crtc)
>>>>>> return;
>>>>>> }
>>>>>>
>>>>>> + if (ctx->mic_bypass && ctx->sysreg &&
>>>>>> regmap_update_bits(ctx->sysreg,
>>>>>> + driver_data->lcdblk_offset,
>>>>>> + 0x1 <<
>>>>>> driver_data->lcdblk_mic_bypass_shift,
>>>>>> + 0x1 <<
>>>>>> driver_data->lcdblk_mic_bypass_shift)) {
>>>>>> + DRM_ERROR("Failed to update sysreg for bypass mic.\n");
>>>>>> + return;
>>>>>> + }
>>>>>
>>>>> It'd better to consider mic path also because mic bypass bit of lcdblk
>>>>> could be true by bootloader. In this case, fimd driver wouldn't do
>>>>> anything if mic_bypass property isn't declared but the mic_bypass bit of
>>>>> lcdblk register is still set to 1.
>>>>
>>>> Actually, I wanted to set the bit on kernel side even though it's not
>>>> assigned from bootloader.
>>>> If the bootloader already set the bit, that means mic will be by-pass
>>>> and we don't care about it from kernel side.
>>>> The option is useful when I want to skip the mic on the kernel side.
>>>>
>>>>>
>>>>> For this, I think you could check lcdblk_mic_pypass_shift to identify
>>>>> whether mic path is supported or not like below,
>>>>> if (ctx->lcdblk_mic_bypass_shift) {
>>>>
>>>> It causes all exynos5 fimd driver skips the mic from display path.
>>>> How about below patch instead of this?
>>>>
>>>> + if (of_property_read_bool(dev->of_node, "samsung,mic-bypass") &&
>>>> + ctx->driver_data->lcdblk_mic_bypass_shift)
>>>> + ctx->mic_bypass = true;
>>>
>>> So let's bypass mic path in default like lcdblk_bypass,
>>
>> I'm not sure why the option is default for exynos542x fimd controller.
>> The reset value of the bit is 0. That means the mic bypass is not a
>> default option on chip-level.
>
> That is because Exynos drm is not ready for mic support fully. As of now,
> Exynos drm uses mic driver only for Exynos5433 SoC but not other SoC.
The MIC is required to process 2560x1440 large image. I think it's not
necessary under full-hd size image.
So, the option is configurable not fixed during display path.
I'll also try to enable the option in exynos5422.
> Do you know that? FIMDBYPASS bit of lcdblk register has 0 - Reserved not FIMD
> bypass(in case of Exynos5422) in default.
>
>>
>>> /* set mic bypass selection */
>>> if (ctx->has_mic_bypass && ctx->sysreg &&
>>> regmap_update_bits(ctx->sysreg,
>>> driver_data->lcdblk_offset,
>>> 0x1 << driver_data->lcdblk_mic_bypass_shift,
>>> 0x1 <<
>>> driver_data->lcdblk_mic_bypass_shift)) {
>>> DRM_ERROR("Failed to update sysreg for mic bypass
>>> setting.\n");
>>> return;
>>> }
>>>
>>> For this, I added a new member, has_mic_by_pass, which means that lcdblk
>>> register has mic_bypass bit like has_vtsel.
>> Hmm. Like lcd_blk_bypass_shift, the option is configurable because
>> mic_by_pass_shift is not zero.
>> I think we don't need the new has_mic_bypass option.
>
> First of all, it seems that adding mic_bypass property to fimd device node is
> really wrong. If so, how can you let fimd driver know whether now SoC has mic
> bypass bit?
Yes. To complete this patch, we should split exynos5420 from exynos5.
Actually, I tried but I was not willing to do that only this bit.
>
>>
>>>
>>> We could consider display path such as mic and mDNIe later using of_graph
>>> concept below,
>>> Documentation/devicetree/bindings/graph.txt
>>
>> Yes. To use the mic on display path, we'll define the graph in the device
>> tree.
>> In case of not using the mic, however, we'll not define any graphs in the
>> dts.
>
> However, fimd driver should know whether now SoC has mic bypass bit because
> fimd driver is used for other SoC also, which have no mic ip.
If you want, I'll make new patch series to support exynos5420 of fimd.
--
Best Regards,
Chanho Park
