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.
The exynos5420 and exynos5422(5800) have a fimd controller not decon
display controller.
>
>>>> + This option is only available since exynos5420.
>>>> - 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.
> /* 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.
>
> 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.
I intended this patch is just providing configurable option from
device tree to skip mic from display path.
--
Best Regards,
Chanho Park
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
