[PATCH 01/11] drm/rockchip: cdn-dp: get rid of drm_edid_raw()

[PATCH 01/11] drm/rockchip: cdn-dp: get rid of drm_edid_raw()

[Jani Nikula]

The dimensions are available in display info, so there's no need for raw
EDID access. While at it, move the debug logging to where the EDID is
actually read.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>

---

Cc: Sandy Huang <hjc@rock-chips.com>
Cc: "Heiko Stübner" <heiko@sntech.de>
Cc: Andy Yan <andy.yan@rock-chips.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Maxime Ripard <mripard@kernel.org>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-rockchip@lists.infradead.org
---
 drivers/gpu/drm/rockchip/cdn-dp-core.c | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/cdn-dp-core.c b/drivers/gpu/drm/rockchip/cdn-dp-core.c
index bd7aa891b839..90913fa26aad 100644
--- a/drivers/gpu/drm/rockchip/cdn-dp-core.c
+++ b/drivers/gpu/drm/rockchip/cdn-dp-core.c
@@ -266,15 +266,6 @@ static int cdn_dp_connector_get_modes(struct drm_connector *connector)
 
 	mutex_lock(&dp->lock);
 
-	if (dp->drm_edid) {
-		/* FIXME: get rid of drm_edid_raw() */
-		const struct edid *edid = drm_edid_raw(dp->drm_edid);
-
-		DRM_DEV_DEBUG_KMS(dp->dev, "got edid: width[%d] x height[%d]\n",
-				  edid->width_cm, edid->height_cm);
-
-	}
-
 	ret = drm_edid_connector_add_modes(connector);
 
 	mutex_unlock(&dp->lock);
@@ -369,6 +360,7 @@ static int cdn_dp_firmware_init(struct cdn_dp_device *dp)
 
 static int cdn_dp_get_sink_capability(struct cdn_dp_device *dp)
 {
+	const struct drm_display_info *info = &dp->connector.display_info;
 	int ret;
 
 	if (!cdn_dp_check_sink_connection(dp))
@@ -386,7 +378,11 @@ static int cdn_dp_get_sink_capability(struct cdn_dp_device *dp)
 					    cdn_dp_get_edid_block, dp);
 	drm_edid_connector_update(&dp->connector, dp->drm_edid);
 
-	dp->sink_has_audio = dp->connector.display_info.has_audio;
+	dp->sink_has_audio = info->has_audio;
+
+	if (dp->drm_edid)
+		DRM_DEV_DEBUG_KMS(dp->dev, "got edid: width[%d] x height[%d]\n",
+				  info->width_mm / 10, info->height_mm / 10);
 
 	return 0;
 }
-- 
2.39.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 04/11] drm/exynos: hdmi: convert to struct drm_edid

[Jani Nikula]

Prefer the struct drm_edid based functions for reading the EDID and
updating the connector.

The functional change is that the CEC physical address gets invalidated
when the EDID could not be read.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>

---

Cc: Inki Dae <inki.dae@samsung.com>
Cc: Seung-Woo Kim <sw0312.kim@samsung.com>
Cc: Kyungmin Park <kyungmin.park@samsung.com>
Cc: Krzysztof Kozlowski <krzk@kernel.org>
Cc: Alim Akhtar <alim.akhtar@samsung.com>
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-samsung-soc@vger.kernel.org
---
 drivers/gpu/drm/exynos/exynos_hdmi.c | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_hdmi.c b/drivers/gpu/drm/exynos/exynos_hdmi.c
index e968824a4c72..9033e8b66816 100644
--- a/drivers/gpu/drm/exynos/exynos_hdmi.c
+++ b/drivers/gpu/drm/exynos/exynos_hdmi.c
@@ -883,27 +883,30 @@ static const struct drm_connector_funcs hdmi_connector_funcs = {
 static int hdmi_get_modes(struct drm_connector *connector)
 {
 	struct hdmi_context *hdata = connector_to_hdmi(connector);
-	struct edid *edid;
+	const struct drm_display_info *info = &connector->display_info;
+	const struct drm_edid *drm_edid;
 	int ret;
 
 	if (!hdata->ddc_adpt)
 		return 0;
 
-	edid = drm_get_edid(connector, hdata->ddc_adpt);
-	if (!edid)
+	drm_edid = drm_edid_read_ddc(connector, hdata->ddc_adpt);
+
+	drm_edid_connector_update(connector, drm_edid);
+
+	cec_notifier_set_phys_addr(hdata->notifier, info->source_physical_address);
+
+	if (!drm_edid)
 		return 0;
 
-	hdata->dvi_mode = !connector->display_info.is_hdmi;
+	hdata->dvi_mode = !info->is_hdmi;
 	DRM_DEV_DEBUG_KMS(hdata->dev, "%s : width[%d] x height[%d]\n",
 			  (hdata->dvi_mode ? "dvi monitor" : "hdmi monitor"),
-			  edid->width_cm, edid->height_cm);
-
-	drm_connector_update_edid_property(connector, edid);
-	cec_notifier_set_phys_addr_from_edid(hdata->notifier, edid);
+			  info->width_mm / 10, info->height_mm / 10);
 
-	ret = drm_add_edid_modes(connector, edid);
+	ret = drm_edid_connector_add_modes(connector);
 
-	kfree(edid);
+	drm_edid_free(drm_edid);
 
 	return ret;
 }
-- 
2.39.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 10/11] drm/imx/tve: convert to struct drm_edid

[Jani Nikula]

Prefer the struct drm_edid based functions for reading the EDID and
updating the connector.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>

---

Cc: Philipp Zabel <p.zabel@pengutronix.de>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Maxime Ripard <mripard@kernel.org>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: Shawn Guo <shawnguo@kernel.org>
Cc: Sascha Hauer <s.hauer@pengutronix.de>
Cc: Pengutronix Kernel Team <kernel@pengutronix.de>
Cc: Fabio Estevam <festevam@gmail.com>
Cc: imx@lists.linux.dev
Cc: linux-arm-kernel@lists.infradead.org
---
 drivers/gpu/drm/imx/ipuv3/imx-tve.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/imx/ipuv3/imx-tve.c b/drivers/gpu/drm/imx/ipuv3/imx-tve.c
index b49bddb85535..29f494bfff67 100644
--- a/drivers/gpu/drm/imx/ipuv3/imx-tve.c
+++ b/drivers/gpu/drm/imx/ipuv3/imx-tve.c
@@ -201,18 +201,16 @@ static int tve_setup_vga(struct imx_tve *tve)
 static int imx_tve_connector_get_modes(struct drm_connector *connector)
 {
 	struct imx_tve *tve = con_to_tve(connector);
-	struct edid *edid;
-	int ret = 0;
+	const struct drm_edid *drm_edid;
+	int ret;
 
 	if (!tve->ddc)
 		return 0;
 
-	edid = drm_get_edid(connector, tve->ddc);
-	if (edid) {
-		drm_connector_update_edid_property(connector, edid);
-		ret = drm_add_edid_modes(connector, edid);
-		kfree(edid);
-	}
+	drm_edid = drm_edid_read_ddc(connector, tve->ddc);
+	drm_edid_connector_update(connector, drm_edid);
+	ret = drm_edid_connector_add_modes(connector);
+	drm_edid_free(drm_edid);
 
 	return ret;
 }
-- 
2.39.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 11/11] drm/imx/ldb: convert to struct drm_edid

[Jani Nikula]

Prefer the struct drm_edid based functions for reading the EDID and
updating the connector.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>

---

Cc: Philipp Zabel <p.zabel@pengutronix.de>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Maxime Ripard <mripard@kernel.org>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: Shawn Guo <shawnguo@kernel.org>
Cc: Sascha Hauer <s.hauer@pengutronix.de>
Cc: Pengutronix Kernel Team <kernel@pengutronix.de>
Cc: Fabio Estevam <festevam@gmail.com>
Cc: imx@lists.linux.dev
Cc: linux-arm-kernel@lists.infradead.org
---
 drivers/gpu/drm/imx/ipuv3/imx-ldb.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/imx/ipuv3/imx-ldb.c b/drivers/gpu/drm/imx/ipuv3/imx-ldb.c
index 71d70194fcbd..793dfb1a3ed0 100644
--- a/drivers/gpu/drm/imx/ipuv3/imx-ldb.c
+++ b/drivers/gpu/drm/imx/ipuv3/imx-ldb.c
@@ -72,7 +72,7 @@ struct imx_ldb_channel {
 	struct device_node *child;
 	struct i2c_adapter *ddc;
 	int chno;
-	void *edid;
+	const struct drm_edid *drm_edid;
 	struct drm_display_mode mode;
 	int mode_valid;
 	u32 bus_format;
@@ -142,15 +142,15 @@ static int imx_ldb_connector_get_modes(struct drm_connector *connector)
 	if (num_modes > 0)
 		return num_modes;
 
-	if (!imx_ldb_ch->edid && imx_ldb_ch->ddc)
-		imx_ldb_ch->edid = drm_get_edid(connector, imx_ldb_ch->ddc);
-
-	if (imx_ldb_ch->edid) {
-		drm_connector_update_edid_property(connector,
-							imx_ldb_ch->edid);
-		num_modes = drm_add_edid_modes(connector, imx_ldb_ch->edid);
+	if (!imx_ldb_ch->drm_edid && imx_ldb_ch->ddc) {
+		imx_ldb_ch->drm_edid = drm_edid_read_ddc(connector,
+							 imx_ldb_ch->ddc);
+		drm_edid_connector_update(connector, imx_ldb_ch->drm_edid);
 	}
 
+	if (imx_ldb_ch->drm_edid)
+		num_modes = drm_edid_connector_add_modes(connector);
+
 	if (imx_ldb_ch->mode_valid) {
 		struct drm_display_mode *mode;
 
@@ -553,7 +553,6 @@ static int imx_ldb_panel_ddc(struct device *dev,
 		struct imx_ldb_channel *channel, struct device_node *child)
 {
 	struct device_node *ddc_node;
-	const u8 *edidp;
 	int ret;
 
 	ddc_node = of_parse_phandle(child, "ddc-i2c-bus", 0);
@@ -567,6 +566,7 @@ static int imx_ldb_panel_ddc(struct device *dev,
 	}
 
 	if (!channel->ddc) {
+		const void *edidp;
 		int edid_len;
 
 		/* if no DDC available, fallback to hardcoded EDID */
@@ -574,8 +574,8 @@ static int imx_ldb_panel_ddc(struct device *dev,
 
 		edidp = of_get_property(child, "edid", &edid_len);
 		if (edidp) {
-			channel->edid = kmemdup(edidp, edid_len, GFP_KERNEL);
-			if (!channel->edid)
+			channel->drm_edid = drm_edid_alloc(edidp, edid_len);
+			if (!channel->drm_edid)
 				return -ENOMEM;
 		} else if (!channel->panel) {
 			/* fallback to display-timings node */
@@ -744,7 +744,7 @@ static void imx_ldb_remove(struct platform_device *pdev)
 	for (i = 0; i < 2; i++) {
 		struct imx_ldb_channel *channel = &imx_ldb->channel[i];
 
-		kfree(channel->edid);
+		drm_edid_free(channel->drm_edid);
 		i2c_put_adapter(channel->ddc);
 	}
 
-- 
2.39.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 10/11] drm/imx/tve: convert to struct drm_edid

[Dmitry Baryshkov]

On Tue, May 14, 2024 at 03:55:16PM +0300, Jani Nikula wrote:
> Prefer the struct drm_edid based functions for reading the EDID and
> updating the connector.
> 
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> 
> ---
> 
> Cc: Philipp Zabel <p.zabel@pengutronix.de>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Maxime Ripard <mripard@kernel.org>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: Shawn Guo <shawnguo@kernel.org>
> Cc: Sascha Hauer <s.hauer@pengutronix.de>
> Cc: Pengutronix Kernel Team <kernel@pengutronix.de>
> Cc: Fabio Estevam <festevam@gmail.com>
> Cc: imx@lists.linux.dev
> Cc: linux-arm-kernel@lists.infradead.org
> ---
>  drivers/gpu/drm/imx/ipuv3/imx-tve.c | 14 ++++++--------
>  1 file changed, 6 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/imx/ipuv3/imx-tve.c b/drivers/gpu/drm/imx/ipuv3/imx-tve.c
> index b49bddb85535..29f494bfff67 100644
> --- a/drivers/gpu/drm/imx/ipuv3/imx-tve.c
> +++ b/drivers/gpu/drm/imx/ipuv3/imx-tve.c
> @@ -201,18 +201,16 @@ static int tve_setup_vga(struct imx_tve *tve)
>  static int imx_tve_connector_get_modes(struct drm_connector *connector)
>  {
>  	struct imx_tve *tve = con_to_tve(connector);
> -	struct edid *edid;
> -	int ret = 0;
> +	const struct drm_edid *drm_edid;
> +	int ret;
>  
>  	if (!tve->ddc)
>  		return 0;
>  
> -	edid = drm_get_edid(connector, tve->ddc);
> -	if (edid) {
> -		drm_connector_update_edid_property(connector, edid);
> -		ret = drm_add_edid_modes(connector, edid);
> -		kfree(edid);
> -	}
> +	drm_edid = drm_edid_read_ddc(connector, tve->ddc);
> +	drm_edid_connector_update(connector, drm_edid);
> +	ret = drm_edid_connector_add_modes(connector);
> +	drm_edid_free(drm_edid);

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

Nit: if you change two last lines, you can drop ret= assignment and use
return drm_edid_connector_add_modes(connector).

Maybe we shoud add cocci rule for such cases.

>  
>  	return ret;
>  }
> -- 
> 2.39.2
> 

-- 
With best wishes
Dmitry

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 11/11] drm/imx/ldb: convert to struct drm_edid

[Dmitry Baryshkov]

On Tue, May 14, 2024 at 03:55:17PM +0300, Jani Nikula wrote:
> Prefer the struct drm_edid based functions for reading the EDID and
> updating the connector.
> 
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> 
> ---
> 
> Cc: Philipp Zabel <p.zabel@pengutronix.de>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Maxime Ripard <mripard@kernel.org>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: Shawn Guo <shawnguo@kernel.org>
> Cc: Sascha Hauer <s.hauer@pengutronix.de>
> Cc: Pengutronix Kernel Team <kernel@pengutronix.de>
> Cc: Fabio Estevam <festevam@gmail.com>
> Cc: imx@lists.linux.dev
> Cc: linux-arm-kernel@lists.infradead.org
> ---
>  drivers/gpu/drm/imx/ipuv3/imx-ldb.c | 24 ++++++++++++------------
>  1 file changed, 12 insertions(+), 12 deletions(-)

Note: https://lore.kernel.org/dri-devel/20240331-drm-imx-cleanup-v2-5-d81c1d1c1026@linaro.org/


Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>


> diff --git a/drivers/gpu/drm/imx/ipuv3/imx-ldb.c b/drivers/gpu/drm/imx/ipuv3/imx-ldb.c
> index 71d70194fcbd..793dfb1a3ed0 100644
> --- a/drivers/gpu/drm/imx/ipuv3/imx-ldb.c
> +++ b/drivers/gpu/drm/imx/ipuv3/imx-ldb.c
> @@ -72,7 +72,7 @@ struct imx_ldb_channel {
>  	struct device_node *child;
>  	struct i2c_adapter *ddc;
>  	int chno;
> -	void *edid;
> +	const struct drm_edid *drm_edid;
>  	struct drm_display_mode mode;
>  	int mode_valid;
>  	u32 bus_format;
> @@ -142,15 +142,15 @@ static int imx_ldb_connector_get_modes(struct drm_connector *connector)
>  	if (num_modes > 0)
>  		return num_modes;
>  
> -	if (!imx_ldb_ch->edid && imx_ldb_ch->ddc)
> -		imx_ldb_ch->edid = drm_get_edid(connector, imx_ldb_ch->ddc);
> -
> -	if (imx_ldb_ch->edid) {
> -		drm_connector_update_edid_property(connector,
> -							imx_ldb_ch->edid);
> -		num_modes = drm_add_edid_modes(connector, imx_ldb_ch->edid);
> +	if (!imx_ldb_ch->drm_edid && imx_ldb_ch->ddc) {
> +		imx_ldb_ch->drm_edid = drm_edid_read_ddc(connector,
> +							 imx_ldb_ch->ddc);
> +		drm_edid_connector_update(connector, imx_ldb_ch->drm_edid);
>  	}
>  
> +	if (imx_ldb_ch->drm_edid)
> +		num_modes = drm_edid_connector_add_modes(connector);
> +
>  	if (imx_ldb_ch->mode_valid) {
>  		struct drm_display_mode *mode;
>  
> @@ -553,7 +553,6 @@ static int imx_ldb_panel_ddc(struct device *dev,
>  		struct imx_ldb_channel *channel, struct device_node *child)
>  {
>  	struct device_node *ddc_node;
> -	const u8 *edidp;
>  	int ret;
>  
>  	ddc_node = of_parse_phandle(child, "ddc-i2c-bus", 0);
> @@ -567,6 +566,7 @@ static int imx_ldb_panel_ddc(struct device *dev,
>  	}
>  
>  	if (!channel->ddc) {
> +		const void *edidp;
>  		int edid_len;
>  
>  		/* if no DDC available, fallback to hardcoded EDID */
> @@ -574,8 +574,8 @@ static int imx_ldb_panel_ddc(struct device *dev,
>  
>  		edidp = of_get_property(child, "edid", &edid_len);
>  		if (edidp) {
> -			channel->edid = kmemdup(edidp, edid_len, GFP_KERNEL);
> -			if (!channel->edid)
> +			channel->drm_edid = drm_edid_alloc(edidp, edid_len);
> +			if (!channel->drm_edid)
>  				return -ENOMEM;
>  		} else if (!channel->panel) {
>  			/* fallback to display-timings node */
> @@ -744,7 +744,7 @@ static void imx_ldb_remove(struct platform_device *pdev)
>  	for (i = 0; i < 2; i++) {
>  		struct imx_ldb_channel *channel = &imx_ldb->channel[i];
>  
> -		kfree(channel->edid);
> +		drm_edid_free(channel->drm_edid);
>  		i2c_put_adapter(channel->ddc);
>  	}
>  
> -- 
> 2.39.2
> 

-- 
With best wishes
Dmitry

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 10/11] drm/imx/tve: convert to struct drm_edid

[Jani Nikula]

On Mon, 20 May 2024, Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote:
> On Tue, May 14, 2024 at 03:55:16PM +0300, Jani Nikula wrote:
>> Prefer the struct drm_edid based functions for reading the EDID and
>> updating the connector.
>> 
>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> 
>> ---
>> 
>> Cc: Philipp Zabel <p.zabel@pengutronix.de>
>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> Cc: Maxime Ripard <mripard@kernel.org>
>> Cc: Thomas Zimmermann <tzimmermann@suse.de>
>> Cc: Shawn Guo <shawnguo@kernel.org>
>> Cc: Sascha Hauer <s.hauer@pengutronix.de>
>> Cc: Pengutronix Kernel Team <kernel@pengutronix.de>
>> Cc: Fabio Estevam <festevam@gmail.com>
>> Cc: imx@lists.linux.dev
>> Cc: linux-arm-kernel@lists.infradead.org
>> ---
>>  drivers/gpu/drm/imx/ipuv3/imx-tve.c | 14 ++++++--------
>>  1 file changed, 6 insertions(+), 8 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/imx/ipuv3/imx-tve.c b/drivers/gpu/drm/imx/ipuv3/imx-tve.c
>> index b49bddb85535..29f494bfff67 100644
>> --- a/drivers/gpu/drm/imx/ipuv3/imx-tve.c
>> +++ b/drivers/gpu/drm/imx/ipuv3/imx-tve.c
>> @@ -201,18 +201,16 @@ static int tve_setup_vga(struct imx_tve *tve)
>>  static int imx_tve_connector_get_modes(struct drm_connector *connector)
>>  {
>>  	struct imx_tve *tve = con_to_tve(connector);
>> -	struct edid *edid;
>> -	int ret = 0;
>> +	const struct drm_edid *drm_edid;
>> +	int ret;
>>  
>>  	if (!tve->ddc)
>>  		return 0;
>>  
>> -	edid = drm_get_edid(connector, tve->ddc);
>> -	if (edid) {
>> -		drm_connector_update_edid_property(connector, edid);
>> -		ret = drm_add_edid_modes(connector, edid);
>> -		kfree(edid);
>> -	}
>> +	drm_edid = drm_edid_read_ddc(connector, tve->ddc);
>> +	drm_edid_connector_update(connector, drm_edid);
>> +	ret = drm_edid_connector_add_modes(connector);
>> +	drm_edid_free(drm_edid);
>
> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>
> Nit: if you change two last lines, you can drop ret= assignment and use
> return drm_edid_connector_add_modes(connector).
>
> Maybe we shoud add cocci rule for such cases.

I think there was a cocci rule like that, but a lot of people (including
yours truly) preferred to keep the assignment, and the patches ended up
in bikeshedding, so the cocci was removed.

My argument is that it's not uncommon to keep adding and removing stuff
while the code evolves, and having to change the return statement is
boring and makes the diff harder to follow. It's a bit like that extra
comma at the end of initialization lists or enumeration definitions.

Others think the code should reflect current state and not prepare for
scenarios that might never arrive.

Both are correct, so it's perfect for never ending bikeshedding. ;)


BR,
Jani.


>
>>  
>>  	return ret;
>>  }
>> -- 
>> 2.39.2
>> 

-- 
Jani Nikula, Intel

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 04/11] drm/exynos: hdmi: convert to struct drm_edid

[Inki Dae]

Hi, Jani Nikula,

Thanks for your contribution and sorry for being late. Below are my comments.

2024년 5월 14일 (화) 오후 9:57, Jani Nikula <jani.nikula@intel.com>님이 작성:
>
> Prefer the struct drm_edid based functions for reading the EDID and
> updating the connector.
>
> The functional change is that the CEC physical address gets invalidated
> when the EDID could not be read.
>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>
> ---
>
> Cc: Inki Dae <inki.dae@samsung.com>
> Cc: Seung-Woo Kim <sw0312.kim@samsung.com>
> Cc: Kyungmin Park <kyungmin.park@samsung.com>
> Cc: Krzysztof Kozlowski <krzk@kernel.org>
> Cc: Alim Akhtar <alim.akhtar@samsung.com>
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-samsung-soc@vger.kernel.org
> ---
>  drivers/gpu/drm/exynos/exynos_hdmi.c | 23 +++++++++++++----------
>  1 file changed, 13 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/exynos/exynos_hdmi.c b/drivers/gpu/drm/exynos/exynos_hdmi.c
> index e968824a4c72..9033e8b66816 100644
> --- a/drivers/gpu/drm/exynos/exynos_hdmi.c
> +++ b/drivers/gpu/drm/exynos/exynos_hdmi.c
> @@ -883,27 +883,30 @@ static const struct drm_connector_funcs hdmi_connector_funcs = {
>  static int hdmi_get_modes(struct drm_connector *connector)
>  {
>         struct hdmi_context *hdata = connector_to_hdmi(connector);
> -       struct edid *edid;
> +       const struct drm_display_info *info = &connector->display_info;
> +       const struct drm_edid *drm_edid;
>         int ret;
>
>         if (!hdata->ddc_adpt)
>                 return 0;
>
> -       edid = drm_get_edid(connector, hdata->ddc_adpt);
> -       if (!edid)
> +       drm_edid = drm_edid_read_ddc(connector, hdata->ddc_adpt);

drm_edid_read_ddc function can return NULL for an error. Could you add
an exception handling?

> +
> +       drm_edid_connector_update(connector, drm_edid);

Ditto. drm_edid_connector_update function can return a negative value
for an error.

> +
> +       cec_notifier_set_phys_addr(hdata->notifier, info->source_physical_address);
> +
> +       if (!drm_edid)
>                 return 0;
>
> -       hdata->dvi_mode = !connector->display_info.is_hdmi;
> +       hdata->dvi_mode = !info->is_hdmi;

Above change wouldn't be related to this patch.

>         DRM_DEV_DEBUG_KMS(hdata->dev, "%s : width[%d] x height[%d]\n",
>                           (hdata->dvi_mode ? "dvi monitor" : "hdmi monitor"),
> -                         edid->width_cm, edid->height_cm);
> -
> -       drm_connector_update_edid_property(connector, edid);
> -       cec_notifier_set_phys_addr_from_edid(hdata->notifier, edid);
> +                         info->width_mm / 10, info->height_mm / 10);

The purpose of this patch would be to replace edid with drm_edid so
how about updating the above change like below?
    drm_edid->edid->width_cm, erm_edid->edid->height_cm);

Thanks,
Inki Dae

>
> -       ret = drm_add_edid_modes(connector, edid);
> +       ret = drm_edid_connector_add_modes(connector);
>
> -       kfree(edid);
> +       drm_edid_free(drm_edid);
>
>         return ret;
>  }
> --
> 2.39.2
>
>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 04/11] drm/exynos: hdmi: convert to struct drm_edid

[Jani Nikula]

On Mon, 10 Jun 2024, Inki Dae <daeinki@gmail.com> wrote:
> Hi, Jani Nikula,
>
> Thanks for your contribution and sorry for being late. Below are my comments.
>
> 2024년 5월 14일 (화) 오후 9:57, Jani Nikula <jani.nikula@intel.com>님이 작성:
>>
>> Prefer the struct drm_edid based functions for reading the EDID and
>> updating the connector.
>>
>> The functional change is that the CEC physical address gets invalidated
>> when the EDID could not be read.
>>
>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>>
>> ---
>>
>> Cc: Inki Dae <inki.dae@samsung.com>
>> Cc: Seung-Woo Kim <sw0312.kim@samsung.com>
>> Cc: Kyungmin Park <kyungmin.park@samsung.com>
>> Cc: Krzysztof Kozlowski <krzk@kernel.org>
>> Cc: Alim Akhtar <alim.akhtar@samsung.com>
>> Cc: linux-arm-kernel@lists.infradead.org
>> Cc: linux-samsung-soc@vger.kernel.org
>> ---
>>  drivers/gpu/drm/exynos/exynos_hdmi.c | 23 +++++++++++++----------
>>  1 file changed, 13 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/exynos/exynos_hdmi.c b/drivers/gpu/drm/exynos/exynos_hdmi.c
>> index e968824a4c72..9033e8b66816 100644
>> --- a/drivers/gpu/drm/exynos/exynos_hdmi.c
>> +++ b/drivers/gpu/drm/exynos/exynos_hdmi.c
>> @@ -883,27 +883,30 @@ static const struct drm_connector_funcs hdmi_connector_funcs = {
>>  static int hdmi_get_modes(struct drm_connector *connector)
>>  {
>>         struct hdmi_context *hdata = connector_to_hdmi(connector);
>> -       struct edid *edid;
>> +       const struct drm_display_info *info = &connector->display_info;
>> +       const struct drm_edid *drm_edid;
>>         int ret;
>>
>>         if (!hdata->ddc_adpt)
>>                 return 0;
>>
>> -       edid = drm_get_edid(connector, hdata->ddc_adpt);
>> -       if (!edid)
>> +       drm_edid = drm_edid_read_ddc(connector, hdata->ddc_adpt);
>
> drm_edid_read_ddc function can return NULL for an error. Could you add
> an exception handling?

If it returns NULL, the below code intentionally passes NULL to
drm_edid_connector_update() to reset the EDID. After that,
cec_notifier_set_phys_addr() also resets the source physical
address. And !drm_edid is handled after that.

>
>> +
>> +       drm_edid_connector_update(connector, drm_edid);
>
> Ditto. drm_edid_connector_update function can return a negative value
> for an error.

Okay.cg

>
>> +
>> +       cec_notifier_set_phys_addr(hdata->notifier, info->source_physical_address);
>> +
>> +       if (!drm_edid)
>>                 return 0;
>>
>> -       hdata->dvi_mode = !connector->display_info.is_hdmi;
>> +       hdata->dvi_mode = !info->is_hdmi;
>
> Above change wouldn't be related to this patch.

Yeah, reuses info which is for info->source_physical_address which would
be too long otherwise. Can add another intermediate patch for that.

>
>>         DRM_DEV_DEBUG_KMS(hdata->dev, "%s : width[%d] x height[%d]\n",
>>                           (hdata->dvi_mode ? "dvi monitor" : "hdmi monitor"),
>> -                         edid->width_cm, edid->height_cm);
>> -
>> -       drm_connector_update_edid_property(connector, edid);
>> -       cec_notifier_set_phys_addr_from_edid(hdata->notifier, edid);
>> +                         info->width_mm / 10, info->height_mm / 10);
>
> The purpose of this patch would be to replace edid with drm_edid so
> how about updating the above change like below?
>     drm_edid->edid->width_cm, erm_edid->edid->height_cm);

struct drm_edid is an opaque type and drm_edid->edid is not available
for drivers to look at.

BR,
Jani.

>
> Thanks,
> Inki Dae
>
>>
>> -       ret = drm_add_edid_modes(connector, edid);
>> +       ret = drm_edid_connector_add_modes(connector);
>>
>> -       kfree(edid);
>> +       drm_edid_free(drm_edid);
>>
>>         return ret;
>>  }
>> --
>> 2.39.2
>>
>>

-- 
Jani Nikula, Intel

Re: [PATCH 01/11] drm/rockchip: cdn-dp: get rid of drm_edid_raw()

[Daniel Vetter]

On Tue, May 14, 2024 at 03:55:07PM +0300, Jani Nikula wrote:
> The dimensions are available in display info, so there's no need for raw
> EDID access. While at it, move the debug logging to where the EDID is
> actually read.
> 
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> 
> ---
> 
> Cc: Sandy Huang <hjc@rock-chips.com>
> Cc: "Heiko Stübner" <heiko@sntech.de>
> Cc: Andy Yan <andy.yan@rock-chips.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Maxime Ripard <mripard@kernel.org>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-rockchip@lists.infradead.org

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> ---
>  drivers/gpu/drm/rockchip/cdn-dp-core.c | 16 ++++++----------
>  1 file changed, 6 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rockchip/cdn-dp-core.c b/drivers/gpu/drm/rockchip/cdn-dp-core.c
> index bd7aa891b839..90913fa26aad 100644
> --- a/drivers/gpu/drm/rockchip/cdn-dp-core.c
> +++ b/drivers/gpu/drm/rockchip/cdn-dp-core.c
> @@ -266,15 +266,6 @@ static int cdn_dp_connector_get_modes(struct drm_connector *connector)
>  
>  	mutex_lock(&dp->lock);
>  
> -	if (dp->drm_edid) {
> -		/* FIXME: get rid of drm_edid_raw() */
> -		const struct edid *edid = drm_edid_raw(dp->drm_edid);
> -
> -		DRM_DEV_DEBUG_KMS(dp->dev, "got edid: width[%d] x height[%d]\n",
> -				  edid->width_cm, edid->height_cm);
> -
> -	}
> -
>  	ret = drm_edid_connector_add_modes(connector);
>  
>  	mutex_unlock(&dp->lock);
> @@ -369,6 +360,7 @@ static int cdn_dp_firmware_init(struct cdn_dp_device *dp)
>  
>  static int cdn_dp_get_sink_capability(struct cdn_dp_device *dp)
>  {
> +	const struct drm_display_info *info = &dp->connector.display_info;
>  	int ret;
>  
>  	if (!cdn_dp_check_sink_connection(dp))
> @@ -386,7 +378,11 @@ static int cdn_dp_get_sink_capability(struct cdn_dp_device *dp)
>  					    cdn_dp_get_edid_block, dp);
>  	drm_edid_connector_update(&dp->connector, dp->drm_edid);
>  
> -	dp->sink_has_audio = dp->connector.display_info.has_audio;
> +	dp->sink_has_audio = info->has_audio;
> +
> +	if (dp->drm_edid)
> +		DRM_DEV_DEBUG_KMS(dp->dev, "got edid: width[%d] x height[%d]\n",
> +				  info->width_mm / 10, info->height_mm / 10);
>  
>  	return 0;
>  }
> -- 
> 2.39.2
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch