| Message ID | 1442907477-3283-1-git-send-email-ykk@rock-chips.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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; >
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
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
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 > > > > "
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;
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. 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). 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(-)