HDMI Aspect Ratio
Hi folks, We're looking at 4K HDMI mode support, and noticed aspect ratio support is in flux. Aspect ratio parsing was added to the mode: https://patchwork.kernel.org/patch/9271401/ But later reverted: https://patchwork.kernel.org/patch/9410765/ We're finding that since aspect ratio information isn't supported, modes conflict, and the userspace isn't able to see the full list of modes. Similarly, there's a case where two modes differ only by aspect_ratio and vrefresh. In drm_edid.c, within drm_display_mode_from_vic_index, newmode->vrefresh is forcibly being cleared to 0. Since refresh rate is forced to 0, and aspect ratio isn’t considered in general, the modes collide and we can’t advertise one of the modes. https://github.com/torvalds/linux/blob/v4.14-rc4/drivers/gpu/drm/drm_edid.c#L3153 Have there been any follow-up discussions on this topic? What is the reason to clear the vrefresh within drm_display_mode_from_vic_index? Thanks, Lloyd Atkinson -- QUALCOMM Canada, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 3/3] drm/msm/dsi: check msm_dsi and dsi pointers before use
On 1/15/2018 9:48 AM, Rob Clark wrote:
> On Fri, Jan 12, 2018 at 3:55 PM, Lloyd Atkinson
> wrote:
>> Move null checks of pointer arguments to the beginning of the
>> modeset init function since they are referenced immediately
>> instead of after they have already been used.
>>
>> Signed-off-by: Lloyd Atkinson
>> ---
>> drivers/gpu/drm/msm/dsi/dsi.c | 22 ++
>> 1 file changed, 10 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/dsi/dsi.c b/drivers/gpu/drm/msm/dsi/dsi.c
>> index 98742d7..be855db 100644
>> --- a/drivers/gpu/drm/msm/dsi/dsi.c
>> +++ b/drivers/gpu/drm/msm/dsi/dsi.c
>> @@ -196,7 +196,7 @@ int msm_dsi_modeset_init(struct msm_dsi *msm_dsi, struct
>> drm_device *dev,
>> struct drm_bridge *ext_bridge;
>> int ret;
>>
>> - if (WARN_ON(!encoder))
>> + if (!encoder || !msm_dsi || !dev)
>
> hmm, the checking if msm_dsi is null later in the fail: case is
> certainly sketchy after we've already deref'd it.. so this looks like
> the right thing. But I'd like to keep the WARN_ON(), since this is a
> case that shouldn't really happen. The WARN_ON() nicely documents
> that none of these parameters are expected to be NULL, and it gives a
> big shouty message to anyone who inadvertently changes something that
> breaks that assumption. Other than that, it looks good.
>
> BR,
> -R
Sure. Do you want to add WARN_ONs to msm_dsi and dev as well?
Thanks,
Lloyd
>
>
>> return -EINVAL;
>>
>> msm_dsi->dev = dev;
>> @@ -245,19 +245,17 @@ int msm_dsi_modeset_init(struct msm_dsi *msm_dsi,
>> struct drm_device *dev,
>>
>> return 0;
>> fail:
>> - if (msm_dsi) {
>> - /* bridge/connector are normally destroyed by drm: */
>> - if (msm_dsi->bridge) {
>> - msm_dsi_manager_bridge_destroy(msm_dsi->bridge);
>> - msm_dsi->bridge = NULL;
>> - }
>> + /* bridge/connector are normally destroyed by drm: */
>> + if (msm_dsi->bridge) {
>> + msm_dsi_manager_bridge_destroy(msm_dsi->bridge);
>> + msm_dsi->bridge = NULL;
>> + }
>>
>> - /* don't destroy connector if we didn't make it */
>> - if (msm_dsi->connector && !msm_dsi->external_bridge)
>> -
>> msm_dsi->connector->funcs->destroy(msm_dsi->connector);
>> + /* don't destroy connector if we didn't make it */
>> + if (msm_dsi->connector && !msm_dsi->external_bridge)
>> + msm_dsi->connector->funcs->destroy(msm_dsi->connector);
>>
>> - msm_dsi->connector = NULL;
>> - }
>> + msm_dsi->connector = NULL;
>>
>> return ret;
>> }
>> --
>> QUALCOMM Canada, on behalf of Qualcomm Innovation Center, Inc. is a member
>> of Code Aurora Forum, hosted by The Linux Foundation
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
>> the body of a message to majord...@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
QUALCOMM Canada, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v2 3/3] drm/msm/dsi: check msm_dsi and dsi pointers before use
Move null checks of pointer arguments to the beginning of the
modeset init function since they are referenced immediately
instead of after they have already been used.
Signed-off-by: Lloyd Atkinson
---
drivers/gpu/drm/msm/dsi/dsi.c | 22 ++
1 file changed, 10 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/msm/dsi/dsi.c b/drivers/gpu/drm/msm/dsi/dsi.c
index 98742d7..ee7e090 100644
--- a/drivers/gpu/drm/msm/dsi/dsi.c
+++ b/drivers/gpu/drm/msm/dsi/dsi.c
@@ -196,7 +196,7 @@ int msm_dsi_modeset_init(struct msm_dsi *msm_dsi, struct
drm_device *dev,
struct drm_bridge *ext_bridge;
int ret;
- if (WARN_ON(!encoder))
+ if (WARN_ON(!encoder) || WARN_ON(!msm_dsi) || WARN_ON(!dev))
return -EINVAL;
msm_dsi->dev = dev;
@@ -245,19 +245,17 @@ int msm_dsi_modeset_init(struct msm_dsi *msm_dsi, struct
drm_device *dev,
return 0;
fail:
- if (msm_dsi) {
- /* bridge/connector are normally destroyed by drm: */
- if (msm_dsi->bridge) {
- msm_dsi_manager_bridge_destroy(msm_dsi->bridge);
- msm_dsi->bridge = NULL;
- }
+ /* bridge/connector are normally destroyed by drm: */
+ if (msm_dsi->bridge) {
+ msm_dsi_manager_bridge_destroy(msm_dsi->bridge);
+ msm_dsi->bridge = NULL;
+ }
- /* don't destroy connector if we didn't make it */
- if (msm_dsi->connector && !msm_dsi->external_bridge)
- msm_dsi->connector->funcs->destroy(msm_dsi->connector);
+ /* don't destroy connector if we didn't make it */
+ if (msm_dsi->connector && !msm_dsi->external_bridge)
+ msm_dsi->connector->funcs->destroy(msm_dsi->connector);
- msm_dsi->connector = NULL;
- }
+ msm_dsi->connector = NULL;
return ret;
}
--
QUALCOMM Canada, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v2 0/3] drm/msm/dsi: improve pointer validation checks
This series improves a few pointer validation checks around the drm/msm/dsi driver. v2 incoporates feedback on patch 1/3 and patch 3/3. Lloyd Atkinson (3): drm/msm/dsi: check for failure on retrieving pll in dsi manager drm/msm/dsi: correct DSI id bounds check during registration drm/msm/dsi: check msm_dsi and dsi pointers before use drivers/gpu/drm/msm/dsi/dsi.c | 22 ++ drivers/gpu/drm/msm/dsi/dsi_manager.c | 6 +- drivers/gpu/drm/msm/dsi/phy/dsi_phy.c | 6 +++--- drivers/gpu/drm/msm/dsi/pll/dsi_pll.c | 2 +- 4 files changed, 19 insertions(+), 17 deletions(-) -- QUALCOMM Canada, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v2 2/3] drm/msm/dsi: correct DSI id bounds check during registration
Check DSI instance id argument against the proper boundary size
to protect against invalid configuration of the DSI id.
Signed-off-by: Lloyd Atkinson
---
drivers/gpu/drm/msm/dsi/dsi_manager.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c
b/drivers/gpu/drm/msm/dsi/dsi_manager.c
index 1a54fd6..4cb1cb6 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_manager.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c
@@ -862,7 +862,7 @@ int msm_dsi_manager_register(struct msm_dsi *msm_dsi)
int id = msm_dsi->id;
int ret;
- if (id > DSI_MAX) {
+ if (id >= DSI_MAX) {
pr_err("%s: invalid id %d\n", __func__, id);
return -EINVAL;
}
--
QUALCOMM Canada, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v2 1/3] drm/msm/dsi: check for failure on retrieving pll in dsi manager
Make msm_dsi_pll_init consistently return an error code instead
of NULL when pll initialization fails so that later pll
retrieval can check against an error code. Add checks for these
failures after retrieval of src_pll to avoid invalid pointer
dereferences later in msm_dsi_pll_get_clk_provider.
Signed-off-by: Lloyd Atkinson
---
drivers/gpu/drm/msm/dsi/dsi_manager.c | 4
drivers/gpu/drm/msm/dsi/phy/dsi_phy.c | 6 +++---
drivers/gpu/drm/msm/dsi/pll/dsi_pll.c | 2 +-
3 files changed, 8 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c
b/drivers/gpu/drm/msm/dsi/dsi_manager.c
index 8552481..1a54fd6 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_manager.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c
@@ -88,6 +88,8 @@ static int dsi_mgr_setup_components(int id)
msm_dsi_phy_set_usecase(msm_dsi->phy, MSM_DSI_PHY_STANDALONE);
src_pll = msm_dsi_phy_get_pll(msm_dsi->phy);
+ if (IS_ERR(src_pll))
+ return PTR_ERR(src_pll);
ret = msm_dsi_host_set_src_pll(msm_dsi->host, src_pll);
} else if (!other_dsi) {
ret = 0;
@@ -116,6 +118,8 @@ static int dsi_mgr_setup_components(int id)
msm_dsi_phy_set_usecase(clk_slave_dsi->phy,
MSM_DSI_PHY_SLAVE);
src_pll = msm_dsi_phy_get_pll(clk_master_dsi->phy);
+ if (IS_ERR(src_pll))
+ return PTR_ERR(src_pll);
ret = msm_dsi_host_set_src_pll(msm_dsi->host, src_pll);
if (ret)
return ret;
diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c
b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c
index 790ca28..c8bfaa7 100644
--- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c
+++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c
@@ -503,10 +503,10 @@ static int dsi_phy_driver_probe(struct platform_device
*pdev)
goto fail;
phy->pll = msm_dsi_pll_init(pdev, phy->cfg->type, phy->id);
- if (!phy->pll)
+ if (IS_ERR_OR_NULL(phy->pll))
dev_info(dev,
- "%s: pll init failed, need separate pll clk driver\n",
- __func__);
+ "%s: pll init failed: %ld, need separate pll clk
driver\n",
+ __func__, PTR_ERR(phy->pll));
dsi_phy_disable_resource(phy);
diff --git a/drivers/gpu/drm/msm/dsi/pll/dsi_pll.c
b/drivers/gpu/drm/msm/dsi/pll/dsi_pll.c
index bc289f5..491f08d 100644
--- a/drivers/gpu/drm/msm/dsi/pll/dsi_pll.c
+++ b/drivers/gpu/drm/msm/dsi/pll/dsi_pll.c
@@ -173,7 +173,7 @@ struct msm_dsi_pll *msm_dsi_pll_init(struct platform_device
*pdev,
if (IS_ERR(pll)) {
dev_err(dev, "%s: failed to init DSI PLL\n", __func__);
- return NULL;
+ return pll;
}
pll->type = type;
--
QUALCOMM Canada, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 2/3] drm/msm/dsi: correct DSI id bounds check during registration
Check DSI instance id argument against the proper boundary size
to protect against invalid configuration of the DSI id.
Signed-off-by: Lloyd Atkinson
---
drivers/gpu/drm/msm/dsi/dsi_manager.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c
b/drivers/gpu/drm/msm/dsi/dsi_manager.c
index d276358..dec2f74 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_manager.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c
@@ -862,7 +862,7 @@ int msm_dsi_manager_register(struct msm_dsi *msm_dsi)
int id = msm_dsi->id;
int ret;
- if (id > DSI_MAX) {
+ if (id >= DSI_MAX) {
pr_err("%s: invalid id %d\n", __func__, id);
return -EINVAL;
}
--
QUALCOMM Canada, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 1/3] drm/msm/dsi: check src_pll for null in dsi manager
Add checks for failure after retrieving the src_pll, since it
may fail. This prevents an invalid pointer dereference later in
msm_dsi_pll_get_clk_provider.
Signed-off-by: Lloyd Atkinson
---
drivers/gpu/drm/msm/dsi/dsi_manager.c | 4
1 file changed, 4 insertions(+)
diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c
b/drivers/gpu/drm/msm/dsi/dsi_manager.c
index 8552481..d276358 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_manager.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c
@@ -88,6 +88,8 @@ static int dsi_mgr_setup_components(int id)
msm_dsi_phy_set_usecase(msm_dsi->phy, MSM_DSI_PHY_STANDALONE);
src_pll = msm_dsi_phy_get_pll(msm_dsi->phy);
+ if (!src_pll)
+ return -EINVAL;
ret = msm_dsi_host_set_src_pll(msm_dsi->host, src_pll);
} else if (!other_dsi) {
ret = 0;
@@ -116,6 +118,8 @@ static int dsi_mgr_setup_components(int id)
msm_dsi_phy_set_usecase(clk_slave_dsi->phy,
MSM_DSI_PHY_SLAVE);
src_pll = msm_dsi_phy_get_pll(clk_master_dsi->phy);
+ if (!src_pll)
+ return -EINVAL;
ret = msm_dsi_host_set_src_pll(msm_dsi->host, src_pll);
if (ret)
return ret;
--
QUALCOMM Canada, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 0/3] drm/msm/dsi: improve pointer validation checks
This series improves a few pointer validation checks around the drm/msm/dsi driver. Lloyd Atkinson (3): drm/msm/dsi: check src_pll for null in dsi manager drm/msm/dsi: correct DSI id bounds check during registration drm/msm/dsi: check msm_dsi and dsi pointers before use drivers/gpu/drm/msm/dsi/dsi.c | 22 ++ drivers/gpu/drm/msm/dsi/dsi_manager.c | 6 +- 2 files changed, 15 insertions(+), 13 deletions(-) -- QUALCOMM Canada, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 3/3] drm/msm/dsi: check msm_dsi and dsi pointers before use
Move null checks of pointer arguments to the beginning of the
modeset init function since they are referenced immediately
instead of after they have already been used.
Signed-off-by: Lloyd Atkinson
---
drivers/gpu/drm/msm/dsi/dsi.c | 22 ++
1 file changed, 10 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/msm/dsi/dsi.c b/drivers/gpu/drm/msm/dsi/dsi.c
index 98742d7..be855db 100644
--- a/drivers/gpu/drm/msm/dsi/dsi.c
+++ b/drivers/gpu/drm/msm/dsi/dsi.c
@@ -196,7 +196,7 @@ int msm_dsi_modeset_init(struct msm_dsi *msm_dsi, struct
drm_device *dev,
struct drm_bridge *ext_bridge;
int ret;
- if (WARN_ON(!encoder))
+ if (!encoder || !msm_dsi || !dev)
return -EINVAL;
msm_dsi->dev = dev;
@@ -245,19 +245,17 @@ int msm_dsi_modeset_init(struct msm_dsi *msm_dsi, struct
drm_device *dev,
return 0;
fail:
- if (msm_dsi) {
- /* bridge/connector are normally destroyed by drm: */
- if (msm_dsi->bridge) {
- msm_dsi_manager_bridge_destroy(msm_dsi->bridge);
- msm_dsi->bridge = NULL;
- }
+ /* bridge/connector are normally destroyed by drm: */
+ if (msm_dsi->bridge) {
+ msm_dsi_manager_bridge_destroy(msm_dsi->bridge);
+ msm_dsi->bridge = NULL;
+ }
- /* don't destroy connector if we didn't make it */
- if (msm_dsi->connector && !msm_dsi->external_bridge)
- msm_dsi->connector->funcs->destroy(msm_dsi->connector);
+ /* don't destroy connector if we didn't make it */
+ if (msm_dsi->connector && !msm_dsi->external_bridge)
+ msm_dsi->connector->funcs->destroy(msm_dsi->connector);
- msm_dsi->connector = NULL;
- }
+ msm_dsi->connector = NULL;
return ret;
}
--
QUALCOMM Canada, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
