Re: [PATCH v5 05/17] drm: bridge: analogix/dp: dynamic parse sync_pol & interlace & dynamic_range

[Krzysztof Kozlowski]

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",
>  >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-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 05/17] drm: bridge: analogix/dp: dynamic parse sync_pol & interlace & dynamic_range

[Krzysztof Kozlowski]

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 
> ---
> 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",
>>>  >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 

Re: [PATCH v5 05/17] drm: bridge: analogix/dp: dynamic parse sync_pol & interlace & dynamic_range

[Yakir Yang]

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 
---
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",
  >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 

Re: [PATCH v5 05/17] drm: bridge: analogix/dp: dynamic parse sync_pol & interlace & dynamic_range

[Krzysztof Kozlowski]

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?

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 = >connector->display_info;
> + struct video_info *video = >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