From mboxrd@z Thu Jan 1 00:00:00 1970 From: Yakir Yang Subject: Re: [PATCH v5 05/17] drm: bridge: analogix/dp: dynamic parse sync_pol & interlace & dynamic_range Date: Wed, 30 Sep 2015 16:20:03 +0800 Message-ID: <560B9B33.2060409@rock-chips.com> References: <1442906428-2609-1-git-send-email-ykk@rock-chips.com> <1442907477-3283-1-git-send-email-ykk@rock-chips.com> <560B73D4.30109@samsung.com> <560B8CF1.7050102@rock-chips.com> <560B9075.1000304@samsung.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============0700629068==" Return-path: In-Reply-To: <560B9075.1000304@samsung.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Krzysztof Kozlowski , Inki Dae , Andrzej Hajda , Joonyoung Shim , Seung-Woo Kim , Kyungmin Park , Jingoo Han , Heiko Stuebner , Mark Yao , Thierry Reding , joe@perches.com, Rob Herring Cc: devicetree@vger.kernel.org, linux-samsung-soc@vger.kernel.org, Russell King , Pawel Moll , Ian Campbell , linux-kernel@vger.kernel.org, emil.l.velikov@gmail.com, dianders@chromium.org, Kishon Vijay Abraham I , linux-rockchip@lists.infradead.org, Kukjin Kim , dri-devel@lists.freedesktop.org, Kumar Gala , ajaynumb@gmail.com, robherring2@gmail.com, Andy Yan , Gustavo Padovan , linux-arm-kernel@lists.infradead.org List-Id: devicetree@vger.kernel.org This is a multi-part message in MIME format. --===============0700629068== Content-Type: multipart/alternative; boundary="------------060104040801090205080705" This is a multi-part message in MIME format. --------------060104040801090205080705 Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit 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 >>>> --- >>>> 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; 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. Thanks, - Yakir > Best regards, > Krzysztof > > > > > --------------060104040801090205080705 Content-Type: text/html; charset=windows-1252 Content-Transfer-Encoding: quoted-printable 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 c=
an 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@rock-chips.com>
---
Changes in v5:
- Switch video timing type to "u32", so driver could use "of_property_rea=
d_u32"
  to get the backword timing values.=20

Okay


Krzysztof suggest me that driver could use
  the "of_property_read_bool" to get backword timing values, but that int=
erfacs
  would modify the original drm_display_mode timing directly (whether tho=
se
  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 =3D
		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 alread=
y
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 =3D !!(mode->flags & DRM_MODE_FL=
AG_NHSYNC);

        // Try to detect the deprecated property, providing
        // the backward compatibility
        of_property_read_u32(dp_node, "hsync-active-high",
                                 &video->h_sync_polarity);   =20

        /*
         * 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"
         */    =20
    }  =20

    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 =3D !!(mode->flags & DRM_MODE_FL=
AG_NHSYNC);

        // Try to detect the deprecated property, providing
        // the backward compatibility
        video->h_sync_polarity =3D
                    of_property_read_bool(dp_node, "hsync-active-high");
  =20

        /*
         * 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"
         */ =20
    }  =20


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,
=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0= const char *propname, u32 len)
{
=A0=A0=A0 =A0=A0=A0 ....
=A0=A0=A0=A0=A0=A0=A0 if (len > prop->length)
=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 return ERR_PTR(-EOVER= FLOW);
=A0=A0=A0 =A0=A0=A0 ...
}

So I though code should be:
=A0=A0=A0 if (of_property_read_bool(dp_node, "hsync-active-high")) =A0=A0=A0 =A0=A0=A0 video->h_sync_polarity =3D true;

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.


Thanks,
- Yakir

Best regards,
Krzysztof

--------------060104040801090205080705-- --===============0700629068== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KZHJpLWRldmVs IG1haWxpbmcgbGlzdApkcmktZGV2ZWxAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHA6Ly9saXN0 cy5mcmVlZGVza3RvcC5vcmcvbWFpbG1hbi9saXN0aW5mby9kcmktZGV2ZWwK --===============0700629068==--