Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1235761 > unrolled thread
| Started by | Krzysztof Kozlowski <k.kozlowski@samsung.com> |
|---|---|
| First post | 2015-09-30 07:40 +0200 |
| Last post | 2015-09-30 11:40 +0200 |
| Articles | 4 — 2 participants |
Back to article view | Back to linux.kernel
This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by
below is the oldest one visible, not the original post.
Re: [PATCH v5 05/17] drm: bridge: analogix/dp: dynamic parse sync_pol & interlace & dynamic_range Krzysztof Kozlowski <k.kozlowski@samsung.com> - 2015-09-30 07:40 +0200
Re: [PATCH v5 05/17] drm: bridge: analogix/dp: dynamic parse sync_pol & interlace & dynamic_range Krzysztof Kozlowski <k.kozlowski@samsung.com> - 2015-09-30 09:40 +0200
Re: [PATCH v5 05/17] drm: bridge: analogix/dp: dynamic parse sync_pol & interlace & dynamic_range Krzysztof Kozlowski <k.kozlowski@samsung.com> - 2015-09-30 10:30 +0200
Re: [PATCH v5 05/17] drm: bridge: analogix/dp: dynamic parse sync_pol & interlace & dynamic_range Yakir Yang <ykk@rock-chips.com> - 2015-09-30 11:40 +0200
| From | Krzysztof Kozlowski <k.kozlowski@samsung.com> |
|---|---|
| Date | 2015-09-30 07:40 +0200 |
| Subject | Re: [PATCH v5 05/17] drm: bridge: analogix/dp: dynamic parse sync_pol & interlace & dynamic_range |
| Message-ID | <qeihs-44N-3@gated-at.bofh.it> |
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@rock-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?
Best regards,
Krzysztof
>
> Changes in v4:
> - Provide backword compatibility with samsung. (Krzysztof)
>
> Changes in v3:
> - Dynamic parse video timing info from struct drm_display_mode and
> struct drm_display_info. (Thierry)
>
> Changes in v2: None
>
> drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 144 +++++++++++++--------
> drivers/gpu/drm/bridge/analogix/analogix_dp_core.h | 8 +-
> drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c | 14 +-
> 3 files changed, 104 insertions(+), 62 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> index 1e3c8d3..6be139b 100644
> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> @@ -890,8 +890,8 @@ static void analogix_dp_commit(struct analogix_dp_device *dp)
> return;
> }
>
> - ret = analogix_dp_set_link_train(dp, dp->video_info->lane_count,
> - dp->video_info->link_rate);
> + ret = analogix_dp_set_link_train(dp, dp->video_info.lane_count,
> + dp->video_info.link_rate);
> if (ret) {
> dev_err(dp->dev, "unable to do link train\n");
> return;
> @@ -1030,6 +1030,85 @@ static void analogix_dp_bridge_disable(struct drm_bridge *bridge)
> dp->dpms_mode = DRM_MODE_DPMS_OFF;
> }
>
> +static void analogix_dp_bridge_mode_set(struct drm_bridge *bridge,
> + struct drm_display_mode *orig_mode,
> + struct drm_display_mode *mode)
> +{
> + struct analogix_dp_device *dp = bridge->driver_private;
> + struct drm_display_info *display_info = &dp->connector->display_info;
> + struct video_info *video = &dp->video_info;
> + struct device_node *dp_node = dp->dev->of_node;
> + int vic;
> +
> + /* Input video interlaces & hsync pol & vsync pol */
> + video->interlaced = !!(mode->flags & DRM_MODE_FLAG_INTERLACE);
> + video->v_sync_polarity = !!(mode->flags & DRM_MODE_FLAG_NVSYNC);
> + video->h_sync_polarity = !!(mode->flags & DRM_MODE_FLAG_NHSYNC);
> +
> + /* Input video dynamic_range & colorimetry */
> + vic = drm_match_cea_mode(mode);
> + if ((vic == 6) || (vic == 7) || (vic == 21) || (vic == 22) ||
> + (vic == 2) || (vic == 3) || (vic == 17) || (vic == 18)) {
> + video->dynamic_range = CEA;
> + video->ycbcr_coeff = COLOR_YCBCR601;
> + } else if (vic) {
> + video->dynamic_range = CEA;
> + video->ycbcr_coeff = COLOR_YCBCR709;
> + } else {
> + video->dynamic_range = VESA;
> + video->ycbcr_coeff = COLOR_YCBCR709;
> + }
> +
> + /* Input vide bpc and color_formats */
> + switch (display_info->bpc) {
> + case 12:
> + video->color_depth = COLOR_12;
> + break;
> + case 10:
> + video->color_depth = COLOR_10;
> + break;
> + case 8:
> + video->color_depth = COLOR_8;
> + break;
> + case 6:
> + video->color_depth = COLOR_6;
> + break;
> + default:
> + video->color_depth = COLOR_8;
> + break;
> + }
> + if (display_info->color_formats & DRM_COLOR_FORMAT_YCRCB444)
> + video->color_space = COLOR_YCBCR444;
> + else if (display_info->color_formats & DRM_COLOR_FORMAT_YCRCB422)
> + video->color_space = COLOR_YCBCR422;
> + else if (display_info->color_formats & DRM_COLOR_FORMAT_RGB444)
> + video->color_space = COLOR_RGB;
> + else
> + video->color_space = COLOR_RGB;
> +
> + /*
> + * NOTE: those property parsing code is used for providing backward
> + * compatibility for samsung platform.
> + * Due to we used the "of_property_read_u32" interfaces, when this
> + * property isn't present, the "video_info" can keep the original
> + * values and wouldn't be modified.
> + */
> + of_property_read_u32(dp_node, "samsung,color-space",
> + &video->color_space);
> + of_property_read_u32(dp_node, "samsung,dynamic-range",
> + &video->dynamic_range);
> + of_property_read_u32(dp_node, "samsung,ycbcr-coeff",
> + &video->ycbcr_coeff);
> + of_property_read_u32(dp_node, "samsung,color-depth",
> + &video->color_depth);
> + of_property_read_u32(dp_node, "hsync-active-high",
> + &video->h_sync_polarity);
> + of_property_read_u32(dp_node, "vsync-active-high",
> + &video->v_sync_polarity);
> + of_property_read_u32(dp_node, "interlaced",
> + &video->interlaced);
> +}
> +
> static void analogix_dp_bridge_nop(struct drm_bridge *bridge)
> {
> /* do nothing */
> @@ -1040,6 +1119,7 @@ static const struct drm_bridge_funcs analogix_dp_bridge_funcs = {
> .disable = analogix_dp_bridge_disable,
> .pre_enable = analogix_dp_bridge_nop,
> .post_disable = analogix_dp_bridge_nop,
> + .mode_set = analogix_dp_bridge_mode_set,
> .attach = analogix_dp_bridge_attach,
> };
>
> @@ -1070,62 +1150,24 @@ static int analogix_dp_create_bridge(struct drm_device *drm_dev,
> return 0;
> }
>
> -static struct video_info *analogix_dp_dt_parse_pdata(struct device *dev)
> +static int analogix_dp_dt_parse_pdata(struct analogix_dp_device *dp)
> {
> - struct device_node *dp_node = dev->of_node;
> - struct video_info *dp_video_config;
> -
> - dp_video_config = devm_kzalloc(dev, sizeof(*dp_video_config),
> - GFP_KERNEL);
> - if (!dp_video_config)
> - return ERR_PTR(-ENOMEM);
> -
> - dp_video_config->h_sync_polarity =
> - of_property_read_bool(dp_node, "hsync-active-high");
> -
> - dp_video_config->v_sync_polarity =
> - of_property_read_bool(dp_node, "vsync-active-high");
> -
> - dp_video_config->interlaced =
> - of_property_read_bool(dp_node, "interlaced");
> -
> - if (of_property_read_u32(dp_node, "samsung,color-space",
> - &dp_video_config->color_space)) {
> - dev_err(dev, "failed to get color-space\n");
> - return ERR_PTR(-EINVAL);
> - }
> -
> - if (of_property_read_u32(dp_node, "samsung,dynamic-range",
> - &dp_video_config->dynamic_range)) {
> - dev_err(dev, "failed to get dynamic-range\n");
> - return ERR_PTR(-EINVAL);
> - }
> -
> - if (of_property_read_u32(dp_node, "samsung,ycbcr-coeff",
> - &dp_video_config->ycbcr_coeff)) {
> - dev_err(dev, "failed to get ycbcr-coeff\n");
> - return ERR_PTR(-EINVAL);
> - }
> -
> - if (of_property_read_u32(dp_node, "samsung,color-depth",
> - &dp_video_config->color_depth)) {
> - dev_err(dev, "failed to get color-depth\n");
> - return ERR_PTR(-EINVAL);
> - }
> + struct device_node *dp_node = dp->dev->of_node;
> + struct video_info *video_info = &dp->video_info
>
> if (of_property_read_u32(dp_node, "samsung,link-rate",
> - &dp_video_config->link_rate)) {
> + &video_info->link_rate)) {
> dev_err(dev, "failed to get link-rate\n");
> - return ERR_PTR(-EINVAL);
> + return -EINVAL;
> }
>
> if (of_property_read_u32(dp_node, "samsung,lane-count",
> - &dp_video_config->lane_count)) {
> + &video_info->lane_count)) {
> dev_err(dev, "failed to get lane-count\n");
> - return ERR_PTR(-EINVAL);
> + return -EINVAL;
> }
>
> - return dp_video_config;
> + return 0;
> }
>
> int analogix_dp_bind(struct device *dev, struct drm_device *drm_dev,
> @@ -1158,9 +1200,9 @@ int analogix_dp_bind(struct device *dev, struct drm_device *drm_dev,
> */
> dp->plat_data = plat_data;
>
> - dp->video_info = analogix_dp_dt_parse_pdata(&pdev->dev);
> - if (IS_ERR(dp->video_info))
> - return PTR_ERR(dp->video_info);
> + ret = analogix_dp_dt_parse_pdata(dp);
> + if (ret)
> + return ret;
>
> dp->phy = devm_phy_get(dp->dev, "dp");
> if (IS_ERR(dp->phy)) {
> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
> index 9a90a18..730486d 100644
> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
> @@ -120,9 +120,9 @@ enum dp_irq_type {
> struct video_info {
> char *name;
>
> - bool h_sync_polarity;
> - bool v_sync_polarity;
> - bool interlaced;
> + u32 h_sync_polarity;
> + u32 v_sync_polarity;
> + u32 interlaced;
>
> enum color_space color_space;
> enum dynamic_range dynamic_range;
> @@ -154,7 +154,7 @@ struct analogix_dp_device {
> unsigned int irq;
> void __iomem *reg_base;
>
> - struct video_info *video_info;
> + struct video_info video_info;
> struct link_train link_train;
> struct work_struct hotplug_work;
> struct phy *phy;
> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
> index a388c0a..861097a 100644
> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
> @@ -1084,15 +1084,15 @@ void analogix_dp_set_video_color_format(struct analogix_dp_device *dp)
> u32 reg;
>
> /* Configure the input color depth, color space, dynamic range */
> - reg = (dp->video_info->dynamic_range << IN_D_RANGE_SHIFT) |
> - (dp->video_info->color_depth << IN_BPC_SHIFT) |
> - (dp->video_info->color_space << IN_COLOR_F_SHIFT);
> + reg = (dp->video_info.dynamic_range << IN_D_RANGE_SHIFT) |
> + (dp->video_info.color_depth << IN_BPC_SHIFT) |
> + (dp->video_info.color_space << IN_COLOR_F_SHIFT);
> writel(reg, dp->reg_base + ANALOGIX_DP_VIDEO_CTL_2);
>
> /* Set Input Color YCbCr Coefficients to ITU601 or ITU709 */
> reg = readl(dp->reg_base + ANALOGIX_DP_VIDEO_CTL_3);
> reg &= ~IN_YC_COEFFI_MASK;
> - if (dp->video_info->ycbcr_coeff)
> + if (dp->video_info.ycbcr_coeff)
> reg |= IN_YC_COEFFI_ITU709;
> else
> reg |= IN_YC_COEFFI_ITU601;
> @@ -1229,17 +1229,17 @@ void analogix_dp_config_video_slave_mode(struct analogix_dp_device *dp)
>
> reg = readl(dp->reg_base + ANALOGIX_DP_VIDEO_CTL_10);
> reg &= ~INTERACE_SCAN_CFG;
> - reg |= (dp->video_info->interlaced << 2);
> + reg |= (dp->video_info.interlaced << 2);
> writel(reg, dp->reg_base + ANALOGIX_DP_VIDEO_CTL_10);
>
> reg = readl(dp->reg_base + ANALOGIX_DP_VIDEO_CTL_10);
> reg &= ~VSYNC_POLARITY_CFG;
> - reg |= (dp->video_info->v_sync_polarity << 1);
> + reg |= (dp->video_info.v_sync_polarity << 1);
> writel(reg, dp->reg_base + ANALOGIX_DP_VIDEO_CTL_10);
>
> reg = readl(dp->reg_base + ANALOGIX_DP_VIDEO_CTL_10);
> reg &= ~HSYNC_POLARITY_CFG;
> - reg |= (dp->video_info->h_sync_polarity << 0);
> + reg |= (dp->video_info.h_sync_polarity << 0);
> writel(reg, dp->reg_base + ANALOGIX_DP_VIDEO_CTL_10);
>
> reg = AUDIO_MODE_SPDIF_MODE | VIDEO_MODE_SLAVE_MODE;
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [next] | [standalone]
| From | Krzysztof Kozlowski <k.kozlowski@samsung.com> |
|---|---|
| Date | 2015-09-30 09:40 +0200 |
| Message-ID | <qek9z-6Kx-1@gated-at.bofh.it> |
| In reply to | #1235761 |
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@rock-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...
Best regards,
Krzysztof
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Krzysztof Kozlowski <k.kozlowski@samsung.com> |
|---|---|
| Date | 2015-09-30 10:30 +0200 |
| Message-ID | <qekW0-7TY-39@gated-at.bofh.it> |
| In reply to | #1235829 |
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@rock-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@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Yakir Yang <ykk@rock-chips.com> |
|---|---|
| Date | 2015-09-30 11:40 +0200 |
| Message-ID | <qem1J-Zq-23@gated-at.bofh.it> |
| In reply to | #1235891 |
Hi Krzysztof,
On 09/30/2015 04:26 PM, Krzysztof Kozlowski wrote:
> 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@rock-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...
Hmm... as I know the timing polarity would influence something like:
- CTS test
- HDCP function
But I though it's more likely that driver would made those functions
failed if
hard code the timing polarity.
And I think it would be better to get timing polarity from
"drm_display_mode".
Caused the analogix_dp driver have called the drm_add_edid_modes() that
function would parse the EDID "detailed timing" block which contained the
correct timing message that panel request.
Besides I see the exynos_fmid driver already setup the timing polarity from
"drm_display_mode", and there is no doubt that exynos dp should set the
same polarity with fmid driver (I guess, just notice that fmid is a kind
of CTRC
driver).
That's to say parsing timing polarity dynamically would give more chances to
make those functions works.
Thanks,
- Yakir
> Best regards,
> Krzysztof
>
>
>
>
"
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web