| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-id: <560B9CAA.9080109@samsung.com>
Date: Wed, 30 Sep 2015 17:26:18 +0900
From: Krzysztof Kozlowski <k.kozlowski@...sung.com>
To: Yakir Yang <ykk@...k-chips.com>, Inki Dae <inki.dae@...sung.com>,
Andrzej Hajda <a.hajda@...sung.com>,
Joonyoung Shim <jy0922.shim@...sung.com>,
Seung-Woo Kim <sw0312.kim@...sung.com>,
Kyungmin Park <kyungmin.park@...sung.com>,
Jingoo Han <jingoohan1@...il.com>,
Heiko Stuebner <heiko@...ech.de>,
Mark Yao <mark.yao@...k-chips.com>,
Thierry Reding <treding@...dia.com>, joe@...ches.com,
Rob Herring <robh+dt@...nel.org>
Cc: David Airlie <airlied@...ux.ie>,
Russell King <linux@....linux.org.uk>, djkurtz@...omium.org,
dianders@...omium.org, Sean Paul <seanpaul@...omium.org>,
Kukjin Kim <kgene@...nel.org>,
Kumar Gala <galak@...eaurora.org>, emil.l.velikov@...il.com,
Ian Campbell <ijc+devicetree@...lion.org.uk>,
Gustavo Padovan <gustavo.padovan@...labora.co.uk>,
Kishon Vijay Abraham I <kishon@...com>,
Pawel Moll <pawel.moll@....com>, ajaynumb@...il.com,
robherring2@...il.com, Andy Yan <andy.yan@...k-chips.com>,
dri-devel@...ts.freedesktop.org, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-samsung-soc@...r.kernel.org,
linux-rockchip@...ts.infradead.org,
linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH v5 05/17] drm: bridge: analogix/dp: dynamic parse sync_pol
& interlace & dynamic_range
On 30.09.2015 17:20, Yakir Yang wrote:
> Hi Krzysztof,
>
> On 09/30/2015 03:34 PM, Krzysztof Kozlowski wrote:
>> On 30.09.2015 16:19, Yakir Yang wrote:
>>> Hi Krzysztof,
>>>
>>> On 09/30/2015 01:32 PM, Krzysztof Kozlowski wrote:
>>>> On 22.09.2015 16:37, Yakir Yang wrote:
>>>>> Both hsync/vsync polarity and interlace mode can be parsed from
>>>>> drm display mode, and dynamic_range and ycbcr_coeff can be judge
>>>>> by the video code.
>>>>>
>>>>> But presumably Exynos still relies on the DT properties, so take
>>>>> good use of mode_fixup() in to achieve the compatibility hacks.
>>>>>
>>>>> Signed-off-by: Yakir Yang <ykk@...k-chips.com>
>>>>> ---
>>>>> Changes in v5:
>>>>> - Switch video timing type to "u32", so driver could use "of_property_read_u32"
>>>>> to get the backword timing values.
>>>> Okay
>>>>
>>>>> Krzysztof suggest me that driver could use
>>>>> the "of_property_read_bool" to get backword timing values, but that interfacs
>>>>> would modify the original drm_display_mode timing directly (whether those
>>>>> properties exists or not).
>>>> Hmm, I don't understand. You have a:
>>>> struct video_info {
>>>> bool h_sync_polarity;
>>>> bool v_sync_polarity;
>>>> bool interlaced;
>>>> };
>>>>
>>>> so what is wrong with:
>>>> dp_video_config->h_sync_polarity =
>>>> of_property_read_bool(dp_node, "hsync-active-high");
>>>>
>>>> Is it exactly the same binding as previously?
>>> Yes, it is the same binding as previously. But just a note that we already
>>> mark those DT binding as deprecated.
>>>
>>> +-interlaced: deprecated prop that can parsed frm drm_display_mode.
>>> +-vsync-active-high: deprecated prop that can parsed frm drm_display_mode.
>>> +-hsync-active-high: deprecated prop that can parsed frm drm_display_mode.
>>>
>>>
>>> For now those values should come from "struct drm_display_mode",
>>> and we already parsed them out from "drm_display_mode" before
>>> driver provide the backward compatibility.
>>>
>>> Let's used the "hsync-active-high" example:
>>> As for now the code would like:
>>> static void analogix_dp_bridge_mode_set(...)
>>> {
>>> // Parsed timing value from "drm_display_mode"
>>> video->h_sync_polarity = !!(mode->flags & DRM_MODE_FLAG_NHSYNC);
>>>
>>> // Try to detect the deprecated property, providing
>>> // the backward compatibility
>>> of_property_read_u32(dp_node, "hsync-active-high",
>>> &video->h_sync_polarity);
>>>
>>> /*
>>> * In this case, if "hsync-active-high" property haven't been
>>> * found, then the video timing "h_sync_polarity" would keep
>>> * no change, keeping the parsed value from "drm_display_mode"
>>> */
>>> }
>>>
>>> But if keep the "of_property_read_bool", then code would like:
>>> static void analogix_dp_bridge_mode_set(...)
>>> {
>>> // Parsed timing value from "drm_display_mode"
>>> video->h_sync_polarity = !!(mode->flags & DRM_MODE_FLAG_NHSYNC);
>>>
>>> // Try to detect the deprecated property, providing
>>> // the backward compatibility
>>> video->h_sync_polarity =
>>> of_property_read_bool(dp_node, "hsync-active-high");
>>>
>>>
>>> /*
>>> * In this case, if "hsync-active-high" property haven't been
>>> * found, then the video timing "h_sync_polarity" would just
>>> * modify to "false". That is the place we don't want, cause
>>> * it would always modify the timing value parsed from
>>> * "drm_display_mode"
>>> */
>>> }
>>>
>> OK, I see the point of overwriting values from drm_display_mode. However
>> I think you changed the binding. I believe the of_property_read_u32()
>> will behave differently for such DTS:
>>
>> exynos_dp {
>> ...
>> hsync-active-high;
>> }
>>
>> It will return -EOVERFLOW which means it would be broken now...
>
> Whoops, thanks for your remind, after try that, I do see over flow error.
> static void *of_find_property_value_of_size(const struct device_node *np,
> const char *propname, u32 len)
> {
> ....
> if (len > prop->length)
> return ERR_PTR(-EOVERFLOW);
> ...
> }
>
> So I though code should be:
> if (of_property_read_bool(dp_node, "hsync-active-high"))
> video->h_sync_polarity = true;
Looks good.
>
> And we can't provide full backward compatibility for this property, cause
> the previous exynos_dp driver would set this timing value to "false" when
> property not defined, but analogix_dp driver keep this timing value
> corresponding to "drm_display_mode" when property not found.
Indeed, the behaviour changes. I don't know if this is important issue...
Best regards,
Krzysztof
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists