Hi,
On Fri, Feb 12, 2016 at 5:04 PM, Inki Dae <inki.dae@xxxxxxxxxxx> wrote:
> Hi Chanho,
>
> 2016년 02월 12일 16:39에 Chanho Park 이(가) 쓴 글:
>> Hi,
>>
>> On Fri, Feb 12, 2016 at 1:56 PM, Inki Dae <inki.dae@xxxxxxxxxxx> 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@xxxxxxxxxxx> wrote:
>>>>> Hi Chanho,
>>>>>
>>>>> 2016년 01월 30일 22:58에 Chanho Park 이(가) 쓴 글:
>>>>>> From: Chanho Park <chanho61.park@xxxxxxxxxxx>
>>>>>>
>>>>>> 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@xxxxxxxxxxx>
>>>>>> Cc: Joonyoung Shim <jy0922.shim@xxxxxxxxxxx>
>>>>>> Cc: Seung-Woo Kim <sw0312.kim@xxxxxxxxxxx>
>>>>>> Signed-off-by: Chanho Park <chanho61.park@xxxxxxxxxxx>
>>>>>> ---
>>>>>> .../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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
