[PATCH 00/11] drm: conversions to struct drm_edid

[PATCH 00/11] drm: conversions to struct drm_edid

[Jani Nikula]

Convert more drivers to struct drm_edid.

Compile tested only.

Jani Nikula (11):
  drm/rockchip: cdn-dp: get rid of drm_edid_raw()
  drm/sti/sti_hdmi: convert to struct drm_edid
  drm/bridge: analogix_dp: convert to struct drm_edid
  drm/exynos: hdmi: convert to struct drm_edid
  drm/hisilicon/hibmc: convert to struct drm_edid
  drm/loongson/7a1000: convert to struct drm_edid
  drm/loongson/7a2000: convert to struct drm_edid
  drm/msm/dp: switch to struct drm_edid
  drm/tegra: convert to struct drm_edid
  drm/imx/tve: convert to struct drm_edid
  drm/imx/ldb: convert to struct drm_edid

 .../drm/bridge/analogix/analogix_dp_core.c    | 15 +++---
 drivers/gpu/drm/exynos/exynos_hdmi.c          | 23 +++++----
 .../gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c  | 17 ++++---
 drivers/gpu/drm/imx/ipuv3/imx-ldb.c           | 24 +++++-----
 drivers/gpu/drm/imx/ipuv3/imx-tve.c           | 14 +++---
 drivers/gpu/drm/loongson/lsdc_output_7a1000.c | 15 +++---
 drivers/gpu/drm/loongson/lsdc_output_7a2000.c | 15 +++---
 drivers/gpu/drm/msm/dp/dp_display.c           | 11 ++---
 drivers/gpu/drm/msm/dp/dp_panel.c             | 47 ++++++-------------
 drivers/gpu/drm/msm/dp/dp_panel.h             |  2 +-
 drivers/gpu/drm/rockchip/cdn-dp-core.c        | 16 +++----
 drivers/gpu/drm/sti/sti_hdmi.c                | 24 ++++++----
 drivers/gpu/drm/tegra/drm.h                   |  2 +-
 drivers/gpu/drm/tegra/output.c                | 29 +++++++-----
 14 files changed, 121 insertions(+), 133 deletions(-)

-- 
2.39.2

[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

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

[PATCH 02/11] drm/sti/sti_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: Alain Volmat <alain.volmat@foss.st.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Maxime Ripard <mripard@kernel.org>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/sti/sti_hdmi.c | 24 ++++++++++++++----------
 1 file changed, 14 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/sti/sti_hdmi.c b/drivers/gpu/drm/sti/sti_hdmi.c
index 500936d5743c..3b62ec2d742f 100644
--- a/drivers/gpu/drm/sti/sti_hdmi.c
+++ b/drivers/gpu/drm/sti/sti_hdmi.c
@@ -974,28 +974,32 @@ static const struct drm_bridge_funcs sti_hdmi_bridge_funcs = {
 
 static int sti_hdmi_connector_get_modes(struct drm_connector *connector)
 {
+	const struct drm_display_info *info = &connector->display_info;
 	struct sti_hdmi_connector *hdmi_connector
 		= to_sti_hdmi_connector(connector);
 	struct sti_hdmi *hdmi = hdmi_connector->hdmi;
-	struct edid *edid;
+	const struct drm_edid *drm_edid;
 	int count;
 
 	DRM_DEBUG_DRIVER("\n");
 
-	edid = drm_get_edid(connector, hdmi->ddc_adapt);
-	if (!edid)
-		goto fail;
+	drm_edid = drm_edid_read_ddc(connector, hdmi->ddc_adapt);
+
+	drm_edid_connector_update(connector, drm_edid);
 
-	cec_notifier_set_phys_addr_from_edid(hdmi->notifier, edid);
+	cec_notifier_set_phys_addr(hdmi->notifier,
+				   connector->display_info.source_physical_address);
+
+	if (!drm_edid)
+		goto fail;
 
-	count = drm_add_edid_modes(connector, edid);
-	drm_connector_update_edid_property(connector, edid);
+	count = drm_edid_connector_add_modes(connector);
 
 	DRM_DEBUG_KMS("%s : %dx%d cm\n",
-		      (connector->display_info.is_hdmi ? "hdmi monitor" : "dvi monitor"),
-		      edid->width_cm, edid->height_cm);
+		      info->is_hdmi ? "hdmi monitor" : "dvi monitor",
+		      info->width_mm / 10, info->height_mm / 10);
 
-	kfree(edid);
+	drm_edid_free(drm_edid);
 	return count;
 
 fail:
-- 
2.39.2

Re: [PATCH 02/11] drm/sti/sti_hdmi: convert to struct drm_edid

[Daniel Vetter]

On Tue, May 14, 2024 at 03:55:08PM +0300, Jani Nikula wrote:
> 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: Alain Volmat <alain.volmat@foss.st.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Maxime Ripard <mripard@kernel.org>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>  drivers/gpu/drm/sti/sti_hdmi.c | 24 ++++++++++++++----------
>  1 file changed, 14 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/sti/sti_hdmi.c b/drivers/gpu/drm/sti/sti_hdmi.c
> index 500936d5743c..3b62ec2d742f 100644
> --- a/drivers/gpu/drm/sti/sti_hdmi.c
> +++ b/drivers/gpu/drm/sti/sti_hdmi.c
> @@ -974,28 +974,32 @@ static const struct drm_bridge_funcs sti_hdmi_bridge_funcs = {
>  
>  static int sti_hdmi_connector_get_modes(struct drm_connector *connector)
>  {
> +	const struct drm_display_info *info = &connector->display_info;
>  	struct sti_hdmi_connector *hdmi_connector
>  		= to_sti_hdmi_connector(connector);
>  	struct sti_hdmi *hdmi = hdmi_connector->hdmi;
> -	struct edid *edid;
> +	const struct drm_edid *drm_edid;
>  	int count;
>  
>  	DRM_DEBUG_DRIVER("\n");
>  
> -	edid = drm_get_edid(connector, hdmi->ddc_adapt);
> -	if (!edid)
> -		goto fail;
> +	drm_edid = drm_edid_read_ddc(connector, hdmi->ddc_adapt);

I think you can use drm_edid_read here since the ddc is correctly set up
with drm_connector_init_with_ddc()

> +
> +	drm_edid_connector_update(connector, drm_edid);
>  
> -	cec_notifier_set_phys_addr_from_edid(hdmi->notifier, edid);
> +	cec_notifier_set_phys_addr(hdmi->notifier,
> +				   connector->display_info.source_physical_address);
> +
> +	if (!drm_edid)
> +		goto fail;

Unless I missed something, all the functions can cope with a NULL edid,
but this jump means in the failure case you'll return stack garbage in
count. Just drop this check?

>  
> -	count = drm_add_edid_modes(connector, edid);
> -	drm_connector_update_edid_property(connector, edid);
> +	count = drm_edid_connector_add_modes(connector);
>  
>  	DRM_DEBUG_KMS("%s : %dx%d cm\n",
> -		      (connector->display_info.is_hdmi ? "hdmi monitor" : "dvi monitor"),
> -		      edid->width_cm, edid->height_cm);
> +		      info->is_hdmi ? "hdmi monitor" : "dvi monitor",
> +		      info->width_mm / 10, info->height_mm / 10);
>  
> -	kfree(edid);
> +	drm_edid_free(drm_edid);
>  	return count;

With the two items addressed:

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

>  
>  fail:
> -- 
> 2.39.2
> 

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

Re: [PATCH 02/11] drm/sti/sti_hdmi: convert to struct drm_edid

[Jani Nikula]

On Mon, 24 Jun 2024, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Tue, May 14, 2024 at 03:55:08PM +0300, Jani Nikula wrote:
>> 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: Alain Volmat <alain.volmat@foss.st.com>
>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> Cc: Maxime Ripard <mripard@kernel.org>
>> Cc: Thomas Zimmermann <tzimmermann@suse.de>
>> ---
>>  drivers/gpu/drm/sti/sti_hdmi.c | 24 ++++++++++++++----------
>>  1 file changed, 14 insertions(+), 10 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/sti/sti_hdmi.c b/drivers/gpu/drm/sti/sti_hdmi.c
>> index 500936d5743c..3b62ec2d742f 100644
>> --- a/drivers/gpu/drm/sti/sti_hdmi.c
>> +++ b/drivers/gpu/drm/sti/sti_hdmi.c
>> @@ -974,28 +974,32 @@ static const struct drm_bridge_funcs sti_hdmi_bridge_funcs = {
>>  
>>  static int sti_hdmi_connector_get_modes(struct drm_connector *connector)
>>  {
>> +	const struct drm_display_info *info = &connector->display_info;
>>  	struct sti_hdmi_connector *hdmi_connector
>>  		= to_sti_hdmi_connector(connector);
>>  	struct sti_hdmi *hdmi = hdmi_connector->hdmi;
>> -	struct edid *edid;
>> +	const struct drm_edid *drm_edid;
>>  	int count;
>>  
>>  	DRM_DEBUG_DRIVER("\n");
>>  
>> -	edid = drm_get_edid(connector, hdmi->ddc_adapt);
>> -	if (!edid)
>> -		goto fail;
>> +	drm_edid = drm_edid_read_ddc(connector, hdmi->ddc_adapt);
>
> I think you can use drm_edid_read here since the ddc is correctly set up
> with drm_connector_init_with_ddc()
>
>> +
>> +	drm_edid_connector_update(connector, drm_edid);
>>  
>> -	cec_notifier_set_phys_addr_from_edid(hdmi->notifier, edid);
>> +	cec_notifier_set_phys_addr(hdmi->notifier,
>> +				   connector->display_info.source_physical_address);
>> +
>> +	if (!drm_edid)
>> +		goto fail;
>
> Unless I missed something, all the functions can cope with a NULL edid,
> but this jump means in the failure case you'll return stack garbage in
> count. Just drop this check?

Reviving an old thread... the goto fail path returns 0, not count.

BR,
Jani.


>
>>  
>> -	count = drm_add_edid_modes(connector, edid);
>> -	drm_connector_update_edid_property(connector, edid);
>> +	count = drm_edid_connector_add_modes(connector);
>>  
>>  	DRM_DEBUG_KMS("%s : %dx%d cm\n",
>> -		      (connector->display_info.is_hdmi ? "hdmi monitor" : "dvi monitor"),
>> -		      edid->width_cm, edid->height_cm);
>> +		      info->is_hdmi ? "hdmi monitor" : "dvi monitor",
>> +		      info->width_mm / 10, info->height_mm / 10);
>>  
>> -	kfree(edid);
>> +	drm_edid_free(drm_edid);
>>  	return count;
>
> With the two items addressed:
>
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>
>>  
>>  fail:
>> -- 
>> 2.39.2
>> 

-- 
Jani Nikula, Intel

Re: [PATCH 02/11] drm/sti/sti_hdmi: convert to struct drm_edid

[Daniel Vetter]

On Thu, Aug 15, 2024 at 03:55:30PM +0300, Jani Nikula wrote:
> On Mon, 24 Jun 2024, Daniel Vetter <daniel@ffwll.ch> wrote:
> > On Tue, May 14, 2024 at 03:55:08PM +0300, Jani Nikula wrote:
> >> 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: Alain Volmat <alain.volmat@foss.st.com>
> >> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >> Cc: Maxime Ripard <mripard@kernel.org>
> >> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> >> ---
> >>  drivers/gpu/drm/sti/sti_hdmi.c | 24 ++++++++++++++----------
> >>  1 file changed, 14 insertions(+), 10 deletions(-)
> >> 
> >> diff --git a/drivers/gpu/drm/sti/sti_hdmi.c b/drivers/gpu/drm/sti/sti_hdmi.c
> >> index 500936d5743c..3b62ec2d742f 100644
> >> --- a/drivers/gpu/drm/sti/sti_hdmi.c
> >> +++ b/drivers/gpu/drm/sti/sti_hdmi.c
> >> @@ -974,28 +974,32 @@ static const struct drm_bridge_funcs sti_hdmi_bridge_funcs = {
> >>  
> >>  static int sti_hdmi_connector_get_modes(struct drm_connector *connector)
> >>  {
> >> +	const struct drm_display_info *info = &connector->display_info;
> >>  	struct sti_hdmi_connector *hdmi_connector
> >>  		= to_sti_hdmi_connector(connector);
> >>  	struct sti_hdmi *hdmi = hdmi_connector->hdmi;
> >> -	struct edid *edid;
> >> +	const struct drm_edid *drm_edid;
> >>  	int count;
> >>  
> >>  	DRM_DEBUG_DRIVER("\n");
> >>  
> >> -	edid = drm_get_edid(connector, hdmi->ddc_adapt);
> >> -	if (!edid)
> >> -		goto fail;
> >> +	drm_edid = drm_edid_read_ddc(connector, hdmi->ddc_adapt);
> >
> > I think you can use drm_edid_read here since the ddc is correctly set up
> > with drm_connector_init_with_ddc()
> >
> >> +
> >> +	drm_edid_connector_update(connector, drm_edid);
> >>  
> >> -	cec_notifier_set_phys_addr_from_edid(hdmi->notifier, edid);
> >> +	cec_notifier_set_phys_addr(hdmi->notifier,
> >> +				   connector->display_info.source_physical_address);
> >> +
> >> +	if (!drm_edid)
> >> +		goto fail;
> >
> > Unless I missed something, all the functions can cope with a NULL edid,
> > but this jump means in the failure case you'll return stack garbage in
> > count. Just drop this check?
> 
> Reviving an old thread... the goto fail path returns 0, not count.

Oops, I stand corrected. With just drm_edid_read used:

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

> 
> BR,
> Jani.
> 
> 
> >
> >>  
> >> -	count = drm_add_edid_modes(connector, edid);
> >> -	drm_connector_update_edid_property(connector, edid);
> >> +	count = drm_edid_connector_add_modes(connector);
> >>  
> >>  	DRM_DEBUG_KMS("%s : %dx%d cm\n",
> >> -		      (connector->display_info.is_hdmi ? "hdmi monitor" : "dvi monitor"),
> >> -		      edid->width_cm, edid->height_cm);
> >> +		      info->is_hdmi ? "hdmi monitor" : "dvi monitor",
> >> +		      info->width_mm / 10, info->height_mm / 10);
> >>  
> >> -	kfree(edid);
> >> +	drm_edid_free(drm_edid);
> >>  	return count;
> >
> > With the two items addressed:
> >
> > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> >
> >>  
> >>  fail:
> >> -- 
> >> 2.39.2
> >> 
> 
> -- 
> Jani Nikula, Intel

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

[PATCH 03/11] drm/bridge: analogix_dp: 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: Andrzej Hajda <andrzej.hajda@intel.com>
Cc: Neil Armstrong <neil.armstrong@linaro.org>
Cc: Robert Foss <rfoss@kernel.org>
Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
Cc: Jonas Karlman <jonas@kwiboo.se>
Cc: Jernej Skrabec <jernej.skrabec@gmail.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Maxime Ripard <mripard@kernel.org>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
---
 .../gpu/drm/bridge/analogix/analogix_dp_core.c    | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
index 7b841232321f..9360b63ad37c 100644
--- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
+++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
@@ -1108,7 +1108,7 @@ static int analogix_dp_prepare_panel(struct analogix_dp_device *dp,
 static int analogix_dp_get_modes(struct drm_connector *connector)
 {
 	struct analogix_dp_device *dp = to_dp(connector);
-	struct edid *edid;
+	const struct drm_edid *drm_edid;
 	int ret, num_modes = 0;
 
 	if (dp->plat_data->panel) {
@@ -1120,12 +1120,13 @@ static int analogix_dp_get_modes(struct drm_connector *connector)
 			return 0;
 		}
 
-		edid = drm_get_edid(connector, &dp->aux.ddc);
-		if (edid) {
-			drm_connector_update_edid_property(&dp->connector,
-							   edid);
-			num_modes += drm_add_edid_modes(&dp->connector, edid);
-			kfree(edid);
+		drm_edid = drm_edid_read_ddc(connector, &dp->aux.ddc);
+
+		drm_edid_connector_update(&dp->connector, drm_edid);
+
+		if (drm_edid) {
+			num_modes += drm_edid_connector_add_modes(&dp->connector);
+			drm_edid_free(drm_edid);
 		}
 
 		ret = analogix_dp_prepare_panel(dp, false, false);
-- 
2.39.2

Re: [PATCH 03/11] drm/bridge: analogix_dp: convert to struct drm_edid

[Dmitry Baryshkov]

On Tue, May 14, 2024 at 03:55:09PM +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: Andrzej Hajda <andrzej.hajda@intel.com>
> Cc: Neil Armstrong <neil.armstrong@linaro.org>
> Cc: Robert Foss <rfoss@kernel.org>
> Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
> Cc: Jonas Karlman <jonas@kwiboo.se>
> Cc: Jernej Skrabec <jernej.skrabec@gmail.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Maxime Ripard <mripard@kernel.org>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>  .../gpu/drm/bridge/analogix/analogix_dp_core.c    | 15 ++++++++-------
>  1 file changed, 8 insertions(+), 7 deletions(-)
> 

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


-- 
With best wishes
Dmitry

[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

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

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

[PATCH 05/11] drm/hisilicon/hibmc: 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: Xinliang Liu <xinliang.liu@linaro.org>
Cc: Tian Tao <tiantao6@hisilicon.com>
Cc: Xinwei Kong <kong.kongxinwei@hisilicon.com>
Cc: Sumit Semwal <sumit.semwal@linaro.org>
Cc: Yongqin Liu <yongqin.liu@linaro.org>
Cc: John Stultz <jstultz@google.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Maxime Ripard <mripard@kernel.org>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
---
 .../gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c    | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c
index 94e2c573a7af..409c551c92af 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c
+++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c
@@ -24,14 +24,16 @@
 
 static int hibmc_connector_get_modes(struct drm_connector *connector)
 {
-	int count;
-	void *edid;
 	struct hibmc_connector *hibmc_connector = to_hibmc_connector(connector);
+	const struct drm_edid *drm_edid;
+	int count;
+
+	drm_edid = drm_edid_read_ddc(connector, &hibmc_connector->adapter);
 
-	edid = drm_get_edid(connector, &hibmc_connector->adapter);
-	if (edid) {
-		drm_connector_update_edid_property(connector, edid);
-		count = drm_add_edid_modes(connector, edid);
+	drm_edid_connector_update(connector, drm_edid);
+
+	if (drm_edid) {
+		count = drm_edid_connector_add_modes(connector);
 		if (count)
 			goto out;
 	}
@@ -42,7 +44,8 @@ static int hibmc_connector_get_modes(struct drm_connector *connector)
 	drm_set_preferred_mode(connector, 1024, 768);
 
 out:
-	kfree(edid);
+	drm_edid_free(drm_edid);
+
 	return count;
 }
 
-- 
2.39.2

Re: [PATCH 05/11] drm/hisilicon/hibmc: convert to struct drm_edid

[Thomas Zimmermann]

Hi

Am 14.05.24 um 14:55 schrieb 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: Xinliang Liu <xinliang.liu@linaro.org>
> Cc: Tian Tao <tiantao6@hisilicon.com>
> Cc: Xinwei Kong <kong.kongxinwei@hisilicon.com>
> Cc: Sumit Semwal <sumit.semwal@linaro.org>
> Cc: Yongqin Liu <yongqin.liu@linaro.org>
> Cc: John Stultz <jstultz@google.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Maxime Ripard <mripard@kernel.org>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>   .../gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c    | 17 ++++++++++-------
>   1 file changed, 10 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c
> index 94e2c573a7af..409c551c92af 100644
> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c
> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c
> @@ -24,14 +24,16 @@
>   
>   static int hibmc_connector_get_modes(struct drm_connector *connector)

Could this function be replaced with a call to 
drm_connector_helper_get_modes()? With the patch, the only difference is 
the call to the _noedid() function, which the DRM core does 
automatically. There will still be a difference in the maximum 
resolution, but standardizing on 1024x768 seems preferable to me.

Best regards
Thomas

>   {
> -	int count;
> -	void *edid;
>   	struct hibmc_connector *hibmc_connector = to_hibmc_connector(connector);
> +	const struct drm_edid *drm_edid;
> +	int count;
> +
> +	drm_edid = drm_edid_read_ddc(connector, &hibmc_connector->adapter);
>   
> -	edid = drm_get_edid(connector, &hibmc_connector->adapter);
> -	if (edid) {
> -		drm_connector_update_edid_property(connector, edid);
> -		count = drm_add_edid_modes(connector, edid);
> +	drm_edid_connector_update(connector, drm_edid);
> +
> +	if (drm_edid) {
> +		count = drm_edid_connector_add_modes(connector);
>   		if (count)
>   			goto out;
>   	}
> @@ -42,7 +44,8 @@ static int hibmc_connector_get_modes(struct drm_connector *connector)
>   	drm_set_preferred_mode(connector, 1024, 768);
>   
>   out:
> -	kfree(edid);
> +	drm_edid_free(drm_edid);
> +
>   	return count;
>   }
>   

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

Re: [PATCH 05/11] drm/hisilicon/hibmc: convert to struct drm_edid

[Jani Nikula]

On Tue, 14 May 2024, Thomas Zimmermann <tzimmermann@suse.de> wrote:
> Hi
>
> Am 14.05.24 um 14:55 schrieb 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: Xinliang Liu <xinliang.liu@linaro.org>
>> Cc: Tian Tao <tiantao6@hisilicon.com>
>> Cc: Xinwei Kong <kong.kongxinwei@hisilicon.com>
>> Cc: Sumit Semwal <sumit.semwal@linaro.org>
>> Cc: Yongqin Liu <yongqin.liu@linaro.org>
>> Cc: John Stultz <jstultz@google.com>
>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> Cc: Maxime Ripard <mripard@kernel.org>
>> Cc: Thomas Zimmermann <tzimmermann@suse.de>
>> ---
>>   .../gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c    | 17 ++++++++++-------
>>   1 file changed, 10 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c
>> index 94e2c573a7af..409c551c92af 100644
>> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c
>> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c
>> @@ -24,14 +24,16 @@
>>   
>>   static int hibmc_connector_get_modes(struct drm_connector *connector)
>
> Could this function be replaced with a call to 
> drm_connector_helper_get_modes()? With the patch, the only difference is 
> the call to the _noedid() function, which the DRM core does 
> automatically. There will still be a difference in the maximum 
> resolution, but standardizing on 1024x768 seems preferable to me.

Looks like it might work, but personally, I'm trying to shy away from
any further cleanups than just switching to struct drm_edid. I've got my
plate pretty full already.

BR,
Jani.


>
> Best regards
> Thomas
>
>>   {
>> -	int count;
>> -	void *edid;
>>   	struct hibmc_connector *hibmc_connector = to_hibmc_connector(connector);
>> +	const struct drm_edid *drm_edid;
>> +	int count;
>> +
>> +	drm_edid = drm_edid_read_ddc(connector, &hibmc_connector->adapter);
>>   
>> -	edid = drm_get_edid(connector, &hibmc_connector->adapter);
>> -	if (edid) {
>> -		drm_connector_update_edid_property(connector, edid);
>> -		count = drm_add_edid_modes(connector, edid);
>> +	drm_edid_connector_update(connector, drm_edid);
>> +
>> +	if (drm_edid) {
>> +		count = drm_edid_connector_add_modes(connector);
>>   		if (count)
>>   			goto out;
>>   	}
>> @@ -42,7 +44,8 @@ static int hibmc_connector_get_modes(struct drm_connector *connector)
>>   	drm_set_preferred_mode(connector, 1024, 768);
>>   
>>   out:
>> -	kfree(edid);
>> +	drm_edid_free(drm_edid);
>> +
>>   	return count;
>>   }
>>   

-- 
Jani Nikula, Intel

Re: [PATCH 05/11] drm/hisilicon/hibmc: convert to struct drm_edid

[Thomas Zimmermann]

Hi

Am 15.05.24 um 14:34 schrieb Jani Nikula:
> On Tue, 14 May 2024, Thomas Zimmermann <tzimmermann@suse.de> wrote:
>> Hi
>>
>> Am 14.05.24 um 14:55 schrieb 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: Xinliang Liu <xinliang.liu@linaro.org>
>>> Cc: Tian Tao <tiantao6@hisilicon.com>
>>> Cc: Xinwei Kong <kong.kongxinwei@hisilicon.com>
>>> Cc: Sumit Semwal <sumit.semwal@linaro.org>
>>> Cc: Yongqin Liu <yongqin.liu@linaro.org>
>>> Cc: John Stultz <jstultz@google.com>
>>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>> Cc: Maxime Ripard <mripard@kernel.org>
>>> Cc: Thomas Zimmermann <tzimmermann@suse.de>
>>> ---
>>>    .../gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c    | 17 ++++++++++-------
>>>    1 file changed, 10 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c
>>> index 94e2c573a7af..409c551c92af 100644
>>> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c
>>> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c
>>> @@ -24,14 +24,16 @@
>>>    
>>>    static int hibmc_connector_get_modes(struct drm_connector *connector)
>> Could this function be replaced with a call to
>> drm_connector_helper_get_modes()? With the patch, the only difference is
>> the call to the _noedid() function, which the DRM core does
>> automatically. There will still be a difference in the maximum
>> resolution, but standardizing on 1024x768 seems preferable to me.
> Looks like it might work, but personally, I'm trying to shy away from
> any further cleanups than just switching to struct drm_edid. I've got my
> plate pretty full already.

In that case

Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>

for the current patch.

Best regards
Thomas

>
> BR,
> Jani.
>
>
>> Best regards
>> Thomas
>>
>>>    {
>>> -	int count;
>>> -	void *edid;
>>>    	struct hibmc_connector *hibmc_connector = to_hibmc_connector(connector);
>>> +	const struct drm_edid *drm_edid;
>>> +	int count;
>>> +
>>> +	drm_edid = drm_edid_read_ddc(connector, &hibmc_connector->adapter);
>>>    
>>> -	edid = drm_get_edid(connector, &hibmc_connector->adapter);
>>> -	if (edid) {
>>> -		drm_connector_update_edid_property(connector, edid);
>>> -		count = drm_add_edid_modes(connector, edid);
>>> +	drm_edid_connector_update(connector, drm_edid);
>>> +
>>> +	if (drm_edid) {
>>> +		count = drm_edid_connector_add_modes(connector);
>>>    		if (count)
>>>    			goto out;
>>>    	}
>>> @@ -42,7 +44,8 @@ static int hibmc_connector_get_modes(struct drm_connector *connector)
>>>    	drm_set_preferred_mode(connector, 1024, 768);
>>>    
>>>    out:
>>> -	kfree(edid);
>>> +	drm_edid_free(drm_edid);
>>> +
>>>    	return count;
>>>    }
>>>    

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

[PATCH 06/11] drm/loongson/7a1000: 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: Sui Jingfeng <suijingfeng@loongson.cn>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Maxime Ripard <mripard@kernel.org>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/loongson/lsdc_output_7a1000.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/loongson/lsdc_output_7a1000.c b/drivers/gpu/drm/loongson/lsdc_output_7a1000.c
index 6fc8dd1c7d9a..600ed4fb0884 100644
--- a/drivers/gpu/drm/loongson/lsdc_output_7a1000.c
+++ b/drivers/gpu/drm/loongson/lsdc_output_7a1000.c
@@ -40,16 +40,15 @@
 
 static int ls7a1000_dpi_connector_get_modes(struct drm_connector *conn)
 {
-	unsigned int num = 0;
-	struct edid *edid;
+	int num;
 
 	if (conn->ddc) {
-		edid = drm_get_edid(conn, conn->ddc);
-		if (edid) {
-			drm_connector_update_edid_property(conn, edid);
-			num = drm_add_edid_modes(conn, edid);
-			kfree(edid);
-		}
+		const struct drm_edid *drm_edid;
+
+		drm_edid = drm_edid_read(conn);
+		drm_edid_connector_update(conn, drm_edid);
+		num = drm_edid_connector_add_modes(conn);
+		drm_edid_free(drm_edid);
 
 		return num;
 	}
-- 
2.39.2

Re: [06/11] drm/loongson/7a1000: convert to struct drm_edid

[Sui Jingfeng]

Hi, Jani


I love your patch, thanks.

On 2024/5/14 20:55, 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>
> ---


Reviewed-by: Sui Jingfeng <sui.jingfeng@linux.dev>

-- 
Best regards,
Sui

[PATCH 07/11] drm/loongson/7a2000: 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: Sui Jingfeng <suijingfeng@loongson.cn>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Maxime Ripard <mripard@kernel.org>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/loongson/lsdc_output_7a2000.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/loongson/lsdc_output_7a2000.c b/drivers/gpu/drm/loongson/lsdc_output_7a2000.c
index ce3dabec887e..2bd797a9b9ff 100644
--- a/drivers/gpu/drm/loongson/lsdc_output_7a2000.c
+++ b/drivers/gpu/drm/loongson/lsdc_output_7a2000.c
@@ -44,16 +44,15 @@
 
 static int ls7a2000_connector_get_modes(struct drm_connector *connector)
 {
-	unsigned int num = 0;
-	struct edid *edid;
+	int num;
 
 	if (connector->ddc) {
-		edid = drm_get_edid(connector, connector->ddc);
-		if (edid) {
-			drm_connector_update_edid_property(connector, edid);
-			num = drm_add_edid_modes(connector, edid);
-			kfree(edid);
-		}
+		const struct drm_edid *drm_edid;
+
+		drm_edid = drm_edid_read(connector);
+		drm_edid_connector_update(connector, drm_edid);
+		num = drm_edid_connector_add_modes(connector);
+		drm_edid_free(drm_edid);
 
 		return num;
 	}
-- 
2.39.2

Re: [07/11] drm/loongson/7a2000: convert to struct drm_edid

[Sui Jingfeng]

Hi, Jani

I love your patch, thanks.


On 2024/5/14 20:55, 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>
> ---


Reviewed-by: Sui Jingfeng <sui.jingfeng@linux.dev>

-- 
Best regards,
Sui

[PATCH 08/11] drm/msm/dp: switch to struct drm_edid

[Jani Nikula]

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

Simplify the flow by updating the EDID property when the EDID is read
instead of at .get_modes.

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

---

Cc: Rob Clark <robdclark@gmail.com>
Cc: Abhinav Kumar <quic_abhinavk@quicinc.com>
Cc: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Cc: Sean Paul <sean@poorly.run>
Cc: Marijn Suijten <marijn.suijten@somainline.org>
Cc: linux-arm-msm@vger.kernel.org
Cc: freedreno@lists.freedesktop.org
---
 drivers/gpu/drm/msm/dp/dp_display.c | 11 +++----
 drivers/gpu/drm/msm/dp/dp_panel.c   | 47 +++++++++--------------------
 drivers/gpu/drm/msm/dp/dp_panel.h   |  2 +-
 3 files changed, 20 insertions(+), 40 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index 672a7ba52eda..9622e58dce3e 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -360,26 +360,25 @@ static int dp_display_send_hpd_notification(struct dp_display_private *dp,
 
 static int dp_display_process_hpd_high(struct dp_display_private *dp)
 {
+	struct drm_connector *connector = dp->dp_display.connector;
+	const struct drm_display_info *info = &connector->display_info;
 	int rc = 0;
-	struct edid *edid;
 
-	rc = dp_panel_read_sink_caps(dp->panel, dp->dp_display.connector);
+	rc = dp_panel_read_sink_caps(dp->panel, connector);
 	if (rc)
 		goto end;
 
 	dp_link_process_request(dp->link);
 
 	if (!dp->dp_display.is_edp)
-		drm_dp_set_subconnector_property(dp->dp_display.connector,
+		drm_dp_set_subconnector_property(connector,
 						 connector_status_connected,
 						 dp->panel->dpcd,
 						 dp->panel->downstream_ports);
 
-	edid = dp->panel->edid;
-
 	dp->dp_display.psr_supported = dp->panel->psr_cap.version && psr_enabled;
 
-	dp->audio_supported = drm_detect_monitor_audio(edid);
+	dp->audio_supported = info->has_audio;
 	dp_panel_handle_sink_request(dp->panel);
 
 	/*
diff --git a/drivers/gpu/drm/msm/dp/dp_panel.c b/drivers/gpu/drm/msm/dp/dp_panel.c
index 07db8f37cd06..a916b5f3b317 100644
--- a/drivers/gpu/drm/msm/dp/dp_panel.c
+++ b/drivers/gpu/drm/msm/dp/dp_panel.c
@@ -108,28 +108,6 @@ static u32 dp_panel_get_supported_bpp(struct dp_panel *dp_panel,
 	return bpp;
 }
 
-static int dp_panel_update_modes(struct drm_connector *connector,
-	struct edid *edid)
-{
-	int rc = 0;
-
-	if (edid) {
-		rc = drm_connector_update_edid_property(connector, edid);
-		if (rc) {
-			DRM_ERROR("failed to update edid property %d\n", rc);
-			return rc;
-		}
-		rc = drm_add_edid_modes(connector, edid);
-		return rc;
-	}
-
-	rc = drm_connector_update_edid_property(connector, NULL);
-	if (rc)
-		DRM_ERROR("failed to update edid property %d\n", rc);
-
-	return rc;
-}
-
 int dp_panel_read_sink_caps(struct dp_panel *dp_panel,
 	struct drm_connector *connector)
 {
@@ -175,12 +153,13 @@ int dp_panel_read_sink_caps(struct dp_panel *dp_panel,
 	if (rc)
 		return rc;
 
-	kfree(dp_panel->edid);
-	dp_panel->edid = NULL;
+	drm_edid_free(dp_panel->drm_edid);
+
+	dp_panel->drm_edid = drm_edid_read_ddc(connector, &panel->aux->ddc);
+
+	drm_edid_connector_update(connector, dp_panel->drm_edid);
 
-	dp_panel->edid = drm_get_edid(connector,
-					      &panel->aux->ddc);
-	if (!dp_panel->edid) {
+	if (!dp_panel->drm_edid) {
 		DRM_ERROR("panel edid read failed\n");
 		/* check edid read fail is due to unplug */
 		if (!dp_catalog_link_is_connected(panel->catalog)) {
@@ -224,13 +203,13 @@ int dp_panel_get_modes(struct dp_panel *dp_panel,
 		return -EINVAL;
 	}
 
-	if (dp_panel->edid)
-		return dp_panel_update_modes(connector, dp_panel->edid);
+	if (dp_panel->drm_edid)
+		return drm_edid_connector_add_modes(connector);
 
 	return 0;
 }
 
-static u8 dp_panel_get_edid_checksum(struct edid *edid)
+static u8 dp_panel_get_edid_checksum(const struct edid *edid)
 {
 	edid += edid->extensions;
 
@@ -249,10 +228,12 @@ void dp_panel_handle_sink_request(struct dp_panel *dp_panel)
 	panel = container_of(dp_panel, struct dp_panel_private, dp_panel);
 
 	if (panel->link->sink_request & DP_TEST_LINK_EDID_READ) {
+		/* FIXME: get rid of drm_edid_raw() */
+		const struct edid *edid = drm_edid_raw(dp_panel->drm_edid);
 		u8 checksum;
 
-		if (dp_panel->edid)
-			checksum = dp_panel_get_edid_checksum(dp_panel->edid);
+		if (edid)
+			checksum = dp_panel_get_edid_checksum(edid);
 		else
 			checksum = dp_panel->connector->real_edid_checksum;
 
@@ -539,5 +520,5 @@ void dp_panel_put(struct dp_panel *dp_panel)
 	if (!dp_panel)
 		return;
 
-	kfree(dp_panel->edid);
+	drm_edid_free(dp_panel->drm_edid);
 }
diff --git a/drivers/gpu/drm/msm/dp/dp_panel.h b/drivers/gpu/drm/msm/dp/dp_panel.h
index 4ea42fa936ae..6722e3923fa5 100644
--- a/drivers/gpu/drm/msm/dp/dp_panel.h
+++ b/drivers/gpu/drm/msm/dp/dp_panel.h
@@ -39,7 +39,7 @@ struct dp_panel {
 	u8 downstream_ports[DP_MAX_DOWNSTREAM_PORTS];
 
 	struct dp_link_info link_info;
-	struct edid *edid;
+	const struct drm_edid *drm_edid;
 	struct drm_connector *connector;
 	struct dp_display_mode dp_mode;
 	struct dp_panel_psr psr_cap;
-- 
2.39.2

Re: [PATCH 08/11] drm/msm/dp: switch to struct drm_edid

[Dmitry Baryshkov]

On Tue, May 14, 2024 at 03:55:14PM +0300, Jani Nikula wrote:
> Prefer the struct drm_edid based functions for reading the EDID and
> updating the connector.
> 
> Simplify the flow by updating the EDID property when the EDID is read
> instead of at .get_modes.
> 
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> 
> ---

The patch looks good to me, I'd like to hear an opinion from Doug (added
to CC).

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

What is the merge strategy for the series? Do you plan to pick up all
the patches to drm-misc or should we pick up individual patches into
driver trees?


> 
> Cc: Rob Clark <robdclark@gmail.com>
> Cc: Abhinav Kumar <quic_abhinavk@quicinc.com>
> Cc: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> Cc: Sean Paul <sean@poorly.run>
> Cc: Marijn Suijten <marijn.suijten@somainline.org>
> Cc: linux-arm-msm@vger.kernel.org
> Cc: freedreno@lists.freedesktop.org
> ---
>  drivers/gpu/drm/msm/dp/dp_display.c | 11 +++----
>  drivers/gpu/drm/msm/dp/dp_panel.c   | 47 +++++++++--------------------
>  drivers/gpu/drm/msm/dp/dp_panel.h   |  2 +-
>  3 files changed, 20 insertions(+), 40 deletions(-)

[skipped]

> @@ -249,10 +228,12 @@ void dp_panel_handle_sink_request(struct dp_panel *dp_panel)
>  	panel = container_of(dp_panel, struct dp_panel_private, dp_panel);
>  
>  	if (panel->link->sink_request & DP_TEST_LINK_EDID_READ) {
> +		/* FIXME: get rid of drm_edid_raw() */

The code here can get use of something like drm_edid_smth_checksum().
'Something', because I could not come up with the word that would make
it clear that it is the declared checksum instead of the actual /
computed one.

> +		const struct edid *edid = drm_edid_raw(dp_panel->drm_edid);
>  		u8 checksum;
>  
> -		if (dp_panel->edid)
> -			checksum = dp_panel_get_edid_checksum(dp_panel->edid);
> +		if (edid)
> +			checksum = dp_panel_get_edid_checksum(edid);
>  		else
>  			checksum = dp_panel->connector->real_edid_checksum;
>  

-- 
With best wishes
Dmitry

Re: [PATCH 08/11] drm/msm/dp: switch to struct drm_edid

[Jani Nikula]

On Sun, 19 May 2024, Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote:
> On Tue, May 14, 2024 at 03:55:14PM +0300, Jani Nikula wrote:
>> Prefer the struct drm_edid based functions for reading the EDID and
>> updating the connector.
>> 
>> Simplify the flow by updating the EDID property when the EDID is read
>> instead of at .get_modes.
>> 
>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> 
>> ---
>
> The patch looks good to me, I'd like to hear an opinion from Doug (added
> to CC).
>
> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

Thanks!

> What is the merge strategy for the series? Do you plan to pick up all
> the patches to drm-misc or should we pick up individual patches into
> driver trees?

I think all of the patches here are connected in theme, but
independent. Either way is fine by me.

>
>
>> 
>> Cc: Rob Clark <robdclark@gmail.com>
>> Cc: Abhinav Kumar <quic_abhinavk@quicinc.com>
>> Cc: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>> Cc: Sean Paul <sean@poorly.run>
>> Cc: Marijn Suijten <marijn.suijten@somainline.org>
>> Cc: linux-arm-msm@vger.kernel.org
>> Cc: freedreno@lists.freedesktop.org
>> ---
>>  drivers/gpu/drm/msm/dp/dp_display.c | 11 +++----
>>  drivers/gpu/drm/msm/dp/dp_panel.c   | 47 +++++++++--------------------
>>  drivers/gpu/drm/msm/dp/dp_panel.h   |  2 +-
>>  3 files changed, 20 insertions(+), 40 deletions(-)
>
> [skipped]
>
>> @@ -249,10 +228,12 @@ void dp_panel_handle_sink_request(struct dp_panel *dp_panel)
>>  	panel = container_of(dp_panel, struct dp_panel_private, dp_panel);
>>  
>>  	if (panel->link->sink_request & DP_TEST_LINK_EDID_READ) {
>> +		/* FIXME: get rid of drm_edid_raw() */
>
> The code here can get use of something like drm_edid_smth_checksum().
> 'Something', because I could not come up with the word that would make
> it clear that it is the declared checksum instead of the actual /
> computed one.

This is an annoying one, to be honest, and linked to the historical fact
that we filter some EDID blocks that have an incorrect checksum.

(Some blocks, yes. We don't filter all blocks, because there are some
nasty docks out there that modify the CTA block but fail to update the
checksum, and filtering the CTA blocks would render the display
useless. So we accept CTA blocks with incorrect checksums. But reject
others. Yay.)

IMO the real fix would be to stop mucking with the EDID, and just expose
it to userspace, warts and all. We could still ignore the EDID blocks
with incorrect checksum while using it ourselves if we want to. And with
that, we could just have a function that checks the last EDID block's
checksum, and stop using this ->real_edid_checksum thing.

Anyway, yes, we could add the function already.

BR,
Jani.

>
>> +		const struct edid *edid = drm_edid_raw(dp_panel->drm_edid);
>>  		u8 checksum;
>>  
>> -		if (dp_panel->edid)
>> -			checksum = dp_panel_get_edid_checksum(dp_panel->edid);
>> +		if (edid)
>> +			checksum = dp_panel_get_edid_checksum(edid);
>>  		else
>>  			checksum = dp_panel->connector->real_edid_checksum;
>>  

-- 
Jani Nikula, Intel

Re: [PATCH 08/11] drm/msm/dp: switch to struct drm_edid

[Dmitry Baryshkov]

On Mon, 20 May 2024 at 15:25, Jani Nikula <jani.nikula@intel.com> wrote:
>
> On Sun, 19 May 2024, Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote:
> > On Tue, May 14, 2024 at 03:55:14PM +0300, Jani Nikula wrote:
> >> Prefer the struct drm_edid based functions for reading the EDID and
> >> updating the connector.
> >>
> >> Simplify the flow by updating the EDID property when the EDID is read
> >> instead of at .get_modes.
> >>
> >> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> >>
> >> ---
> >
> > The patch looks good to me, I'd like to hear an opinion from Doug (added
> > to CC).
> >
> > Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>
> Thanks!
>
> > What is the merge strategy for the series? Do you plan to pick up all
> > the patches to drm-misc or should we pick up individual patches into
> > driver trees?
>
> I think all of the patches here are connected in theme, but
> independent. Either way is fine by me.
>
> >
> >
> >>
> >> Cc: Rob Clark <robdclark@gmail.com>
> >> Cc: Abhinav Kumar <quic_abhinavk@quicinc.com>
> >> Cc: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> >> Cc: Sean Paul <sean@poorly.run>
> >> Cc: Marijn Suijten <marijn.suijten@somainline.org>
> >> Cc: linux-arm-msm@vger.kernel.org
> >> Cc: freedreno@lists.freedesktop.org
> >> ---
> >>  drivers/gpu/drm/msm/dp/dp_display.c | 11 +++----
> >>  drivers/gpu/drm/msm/dp/dp_panel.c   | 47 +++++++++--------------------
> >>  drivers/gpu/drm/msm/dp/dp_panel.h   |  2 +-
> >>  3 files changed, 20 insertions(+), 40 deletions(-)
> >
> > [skipped]
> >
> >> @@ -249,10 +228,12 @@ void dp_panel_handle_sink_request(struct dp_panel *dp_panel)
> >>      panel = container_of(dp_panel, struct dp_panel_private, dp_panel);
> >>
> >>      if (panel->link->sink_request & DP_TEST_LINK_EDID_READ) {
> >> +            /* FIXME: get rid of drm_edid_raw() */
> >
> > The code here can get use of something like drm_edid_smth_checksum().
> > 'Something', because I could not come up with the word that would make
> > it clear that it is the declared checksum instead of the actual /
> > computed one.
>
> This is an annoying one, to be honest, and linked to the historical fact
> that we filter some EDID blocks that have an incorrect checksum.

It is a part of the DP test suite if I remember correctly.

>
> (Some blocks, yes. We don't filter all blocks, because there are some
> nasty docks out there that modify the CTA block but fail to update the
> checksum, and filtering the CTA blocks would render the display
> useless. So we accept CTA blocks with incorrect checksums. But reject
> others. Yay.)
>
> IMO the real fix would be to stop mucking with the EDID, and just expose
> it to userspace, warts and all. We could still ignore the EDID blocks
> with incorrect checksum while using it ourselves if we want to. And with
> that, we could just have a function that checks the last EDID block's
> checksum, and stop using this ->real_edid_checksum thing.
>
> Anyway, yes, we could add the function already.
>
> BR,
> Jani.
>
> >
> >> +            const struct edid *edid = drm_edid_raw(dp_panel->drm_edid);
> >>              u8 checksum;
> >>
> >> -            if (dp_panel->edid)
> >> -                    checksum = dp_panel_get_edid_checksum(dp_panel->edid);
> >> +            if (edid)
> >> +                    checksum = dp_panel_get_edid_checksum(edid);
> >>              else
> >>                      checksum = dp_panel->connector->real_edid_checksum;
> >>
>
> --
> Jani Nikula, Intel



-- 
With best wishes
Dmitry

Re: [PATCH 08/11] drm/msm/dp: switch to struct drm_edid

[Doug Anderson]

Hi,

On Sun, May 19, 2024 at 2:01 AM Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>
> On Tue, May 14, 2024 at 03:55:14PM +0300, Jani Nikula wrote:
> > Prefer the struct drm_edid based functions for reading the EDID and
> > updating the connector.
> >
> > Simplify the flow by updating the EDID property when the EDID is read
> > instead of at .get_modes.
> >
> > Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> >
> > ---
>
> The patch looks good to me, I'd like to hear an opinion from Doug (added
> to CC).
>
> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>
> What is the merge strategy for the series? Do you plan to pick up all
> the patches to drm-misc or should we pick up individual patches into
> driver trees?

I'm not sure I have too much to add here aside from what you guys have
already talked about. I'm OK with:

Reviewed-by: Douglas Anderson <dianders@chromium.org>

Re: [PATCH 08/11] drm/msm/dp: switch to struct drm_edid

[Jani Nikula]

On Mon, 20 May 2024, Doug Anderson <dianders@chromium.org> wrote:
> Hi,
>
> On Sun, May 19, 2024 at 2:01 AM Dmitry Baryshkov
> <dmitry.baryshkov@linaro.org> wrote:
>>
>> On Tue, May 14, 2024 at 03:55:14PM +0300, Jani Nikula wrote:
>> > Prefer the struct drm_edid based functions for reading the EDID and
>> > updating the connector.
>> >
>> > Simplify the flow by updating the EDID property when the EDID is read
>> > instead of at .get_modes.
>> >
>> > Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> >
>> > ---
>>
>> The patch looks good to me, I'd like to hear an opinion from Doug (added
>> to CC).
>>
>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>
>> What is the merge strategy for the series? Do you plan to pick up all
>> the patches to drm-misc or should we pick up individual patches into
>> driver trees?
>
> I'm not sure I have too much to add here aside from what you guys have
> already talked about. I'm OK with:
>
> Reviewed-by: Douglas Anderson <dianders@chromium.org>

I assume you'll want to pick this up for msm instead of me merging to
drm-misc.

BR,
Jani.

-- 
Jani Nikula, Intel

Re: [PATCH 08/11] drm/msm/dp: switch to struct drm_edid

[Dmitry Baryshkov]

On Thu, 30 May 2024 at 15:45, Jani Nikula <jani.nikula@intel.com> wrote:
>
> On Mon, 20 May 2024, Doug Anderson <dianders@chromium.org> wrote:
> > Hi,
> >
> > On Sun, May 19, 2024 at 2:01 AM Dmitry Baryshkov
> > <dmitry.baryshkov@linaro.org> wrote:
> >>
> >> On Tue, May 14, 2024 at 03:55:14PM +0300, Jani Nikula wrote:
> >> > Prefer the struct drm_edid based functions for reading the EDID and
> >> > updating the connector.
> >> >
> >> > Simplify the flow by updating the EDID property when the EDID is read
> >> > instead of at .get_modes.
> >> >
> >> > Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> >> >
> >> > ---
> >>
> >> The patch looks good to me, I'd like to hear an opinion from Doug (added
> >> to CC).
> >>
> >> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> >>
> >> What is the merge strategy for the series? Do you plan to pick up all
> >> the patches to drm-misc or should we pick up individual patches into
> >> driver trees?
> >
> > I'm not sure I have too much to add here aside from what you guys have
> > already talked about. I'm OK with:
> >
> > Reviewed-by: Douglas Anderson <dianders@chromium.org>
>
> I assume you'll want to pick this up for msm instead of me merging to
> drm-misc.

Yes, it's on my todo list.

-- 
With best wishes
Dmitry

[PATCH 09/11] drm/tegra: 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: Thierry Reding <thierry.reding@gmail.com>
Cc: Mikko Perttunen <mperttunen@nvidia.com>
Cc: Jonathan Hunter <jonathanh@nvidia.com>
Cc: linux-tegra@vger.kernel.org
---
 drivers/gpu/drm/tegra/drm.h    |  2 +-
 drivers/gpu/drm/tegra/output.c | 29 +++++++++++++++++------------
 2 files changed, 18 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/tegra/drm.h b/drivers/gpu/drm/tegra/drm.h
index 682011166a8f..2f3781e04b0a 100644
--- a/drivers/gpu/drm/tegra/drm.h
+++ b/drivers/gpu/drm/tegra/drm.h
@@ -133,7 +133,7 @@ struct tegra_output {
 	struct drm_bridge *bridge;
 	struct drm_panel *panel;
 	struct i2c_adapter *ddc;
-	const struct edid *edid;
+	const struct drm_edid *drm_edid;
 	struct cec_notifier *cec;
 	unsigned int hpd_irq;
 	struct gpio_desc *hpd_gpio;
diff --git a/drivers/gpu/drm/tegra/output.c b/drivers/gpu/drm/tegra/output.c
index 4da3c3d1abbc..e6b5863fec71 100644
--- a/drivers/gpu/drm/tegra/output.c
+++ b/drivers/gpu/drm/tegra/output.c
@@ -21,7 +21,7 @@
 int tegra_output_connector_get_modes(struct drm_connector *connector)
 {
 	struct tegra_output *output = connector_to_output(connector);
-	struct edid *edid = NULL;
+	const struct drm_edid *drm_edid;
 	int err = 0;
 
 	/*
@@ -34,18 +34,17 @@ int tegra_output_connector_get_modes(struct drm_connector *connector)
 			return err;
 	}
 
-	if (output->edid)
-		edid = kmemdup(output->edid, sizeof(*edid), GFP_KERNEL);
+	if (output->drm_edid)
+		drm_edid = drm_edid_dup(output->drm_edid);
 	else if (output->ddc)
-		edid = drm_get_edid(connector, output->ddc);
+		drm_edid = drm_edid_read_ddc(connector, output->ddc);
 
-	cec_notifier_set_phys_addr_from_edid(output->cec, edid);
-	drm_connector_update_edid_property(connector, edid);
+	drm_edid_connector_update(connector, drm_edid);
+	cec_notifier_set_phys_addr(output->cec,
+				   connector->display_info.source_physical_address);
 
-	if (edid) {
-		err = drm_add_edid_modes(connector, edid);
-		kfree(edid);
-	}
+	err = drm_edid_connector_add_modes(connector);
+	drm_edid_free(drm_edid);
 
 	return err;
 }
@@ -98,6 +97,7 @@ static irqreturn_t hpd_irq(int irq, void *data)
 int tegra_output_probe(struct tegra_output *output)
 {
 	struct device_node *ddc, *panel;
+	const void *edid;
 	unsigned long flags;
 	int err, size;
 
@@ -124,8 +124,6 @@ int tegra_output_probe(struct tegra_output *output)
 			return PTR_ERR(output->panel);
 	}
 
-	output->edid = of_get_property(output->of_node, "nvidia,edid", &size);
-
 	ddc = of_parse_phandle(output->of_node, "nvidia,ddc-i2c-bus", 0);
 	if (ddc) {
 		output->ddc = of_get_i2c_adapter_by_node(ddc);
@@ -137,6 +135,9 @@ int tegra_output_probe(struct tegra_output *output)
 		}
 	}
 
+	edid = of_get_property(output->of_node, "nvidia,edid", &size);
+	output->drm_edid = drm_edid_alloc(edid, size);
+
 	output->hpd_gpio = devm_fwnode_gpiod_get(output->dev,
 					of_fwnode_handle(output->of_node),
 					"nvidia,hpd",
@@ -187,6 +188,8 @@ int tegra_output_probe(struct tegra_output *output)
 	if (output->ddc)
 		i2c_put_adapter(output->ddc);
 
+	drm_edid_free(output->drm_edid);
+
 	return err;
 }
 
@@ -197,6 +200,8 @@ void tegra_output_remove(struct tegra_output *output)
 
 	if (output->ddc)
 		i2c_put_adapter(output->ddc);
+
+	drm_edid_free(output->drm_edid);
 }
 
 int tegra_output_init(struct drm_device *drm, struct tegra_output *output)
-- 
2.39.2

Re: [PATCH 09/11] drm/tegra: convert to struct drm_edid

[Daniel Vetter]

On Tue, May 14, 2024 at 03:55:15PM +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: Thierry Reding <thierry.reding@gmail.com>
> Cc: Mikko Perttunen <mperttunen@nvidia.com>
> Cc: Jonathan Hunter <jonathanh@nvidia.com>
> Cc: linux-tegra@vger.kernel.org
> ---
>  drivers/gpu/drm/tegra/drm.h    |  2 +-
>  drivers/gpu/drm/tegra/output.c | 29 +++++++++++++++++------------
>  2 files changed, 18 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/tegra/drm.h b/drivers/gpu/drm/tegra/drm.h
> index 682011166a8f..2f3781e04b0a 100644
> --- a/drivers/gpu/drm/tegra/drm.h
> +++ b/drivers/gpu/drm/tegra/drm.h
> @@ -133,7 +133,7 @@ struct tegra_output {
>  	struct drm_bridge *bridge;
>  	struct drm_panel *panel;
>  	struct i2c_adapter *ddc;
> -	const struct edid *edid;
> +	const struct drm_edid *drm_edid;
>  	struct cec_notifier *cec;
>  	unsigned int hpd_irq;
>  	struct gpio_desc *hpd_gpio;
> diff --git a/drivers/gpu/drm/tegra/output.c b/drivers/gpu/drm/tegra/output.c
> index 4da3c3d1abbc..e6b5863fec71 100644
> --- a/drivers/gpu/drm/tegra/output.c
> +++ b/drivers/gpu/drm/tegra/output.c
> @@ -21,7 +21,7 @@
>  int tegra_output_connector_get_modes(struct drm_connector *connector)
>  {
>  	struct tegra_output *output = connector_to_output(connector);
> -	struct edid *edid = NULL;
> +	const struct drm_edid *drm_edid;
>  	int err = 0;
>  
>  	/*
> @@ -34,18 +34,17 @@ int tegra_output_connector_get_modes(struct drm_connector *connector)
>  			return err;
>  	}
>  
> -	if (output->edid)
> -		edid = kmemdup(output->edid, sizeof(*edid), GFP_KERNEL);
> +	if (output->drm_edid)
> +		drm_edid = drm_edid_dup(output->drm_edid);
>  	else if (output->ddc)
> -		edid = drm_get_edid(connector, output->ddc);
> +		drm_edid = drm_edid_read_ddc(connector, output->ddc);

Annoyingly the dsi output is the only one not using
drm_connector_init_with_ddc() despite that I think it could, so we can't
yet use drm_edid_read here ...

>  
> -	cec_notifier_set_phys_addr_from_edid(output->cec, edid);
> -	drm_connector_update_edid_property(connector, edid);
> +	drm_edid_connector_update(connector, drm_edid);
> +	cec_notifier_set_phys_addr(output->cec,
> +				   connector->display_info.source_physical_address);
>  
> -	if (edid) {
> -		err = drm_add_edid_modes(connector, edid);
> -		kfree(edid);
> -	}
> +	err = drm_edid_connector_add_modes(connector);
> +	drm_edid_free(drm_edid);
>  
>  	return err;
>  }
> @@ -98,6 +97,7 @@ static irqreturn_t hpd_irq(int irq, void *data)
>  int tegra_output_probe(struct tegra_output *output)
>  {
>  	struct device_node *ddc, *panel;
> +	const void *edid;
>  	unsigned long flags;
>  	int err, size;
>  
> @@ -124,8 +124,6 @@ int tegra_output_probe(struct tegra_output *output)
>  			return PTR_ERR(output->panel);
>  	}
>  
> -	output->edid = of_get_property(output->of_node, "nvidia,edid", &size);
> -

I think you can/should also delete tegra_output.edid completely, since not
doing that freaked me out a bit until I checked everything :-) At least if
I didn't git grep the wrong regex.

With that bikeshed addressed:

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

>  	ddc = of_parse_phandle(output->of_node, "nvidia,ddc-i2c-bus", 0);
>  	if (ddc) {
>  		output->ddc = of_get_i2c_adapter_by_node(ddc);
> @@ -137,6 +135,9 @@ int tegra_output_probe(struct tegra_output *output)
>  		}
>  	}
>  
> +	edid = of_get_property(output->of_node, "nvidia,edid", &size);
> +	output->drm_edid = drm_edid_alloc(edid, size);
> +
>  	output->hpd_gpio = devm_fwnode_gpiod_get(output->dev,
>  					of_fwnode_handle(output->of_node),
>  					"nvidia,hpd",
> @@ -187,6 +188,8 @@ int tegra_output_probe(struct tegra_output *output)
>  	if (output->ddc)
>  		i2c_put_adapter(output->ddc);
>  
> +	drm_edid_free(output->drm_edid);
> +
>  	return err;
>  }
>  
> @@ -197,6 +200,8 @@ void tegra_output_remove(struct tegra_output *output)
>  
>  	if (output->ddc)
>  		i2c_put_adapter(output->ddc);
> +
> +	drm_edid_free(output->drm_edid);
>  }
>  
>  int tegra_output_init(struct drm_device *drm, struct tegra_output *output)
> -- 
> 2.39.2
> 

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

Re: [PATCH 09/11] drm/tegra: convert to struct drm_edid

[Jani Nikula]

On Mon, 24 Jun 2024, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Tue, May 14, 2024 at 03:55:15PM +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: Thierry Reding <thierry.reding@gmail.com>
>> Cc: Mikko Perttunen <mperttunen@nvidia.com>
>> Cc: Jonathan Hunter <jonathanh@nvidia.com>
>> Cc: linux-tegra@vger.kernel.org
>> ---
>>  drivers/gpu/drm/tegra/drm.h    |  2 +-
>>  drivers/gpu/drm/tegra/output.c | 29 +++++++++++++++++------------
>>  2 files changed, 18 insertions(+), 13 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/tegra/drm.h b/drivers/gpu/drm/tegra/drm.h
>> index 682011166a8f..2f3781e04b0a 100644
>> --- a/drivers/gpu/drm/tegra/drm.h
>> +++ b/drivers/gpu/drm/tegra/drm.h
>> @@ -133,7 +133,7 @@ struct tegra_output {
>>  	struct drm_bridge *bridge;
>>  	struct drm_panel *panel;
>>  	struct i2c_adapter *ddc;
>> -	const struct edid *edid;
>> +	const struct drm_edid *drm_edid;

I keep reviving old stuff, sorry.

Please take note here, removing the edid member.

>>  	struct cec_notifier *cec;
>>  	unsigned int hpd_irq;
>>  	struct gpio_desc *hpd_gpio;
>> diff --git a/drivers/gpu/drm/tegra/output.c b/drivers/gpu/drm/tegra/output.c
>> index 4da3c3d1abbc..e6b5863fec71 100644
>> --- a/drivers/gpu/drm/tegra/output.c
>> +++ b/drivers/gpu/drm/tegra/output.c
>> @@ -21,7 +21,7 @@
>>  int tegra_output_connector_get_modes(struct drm_connector *connector)
>>  {
>>  	struct tegra_output *output = connector_to_output(connector);
>> -	struct edid *edid = NULL;
>> +	const struct drm_edid *drm_edid;
>>  	int err = 0;
>>  
>>  	/*
>> @@ -34,18 +34,17 @@ int tegra_output_connector_get_modes(struct drm_connector *connector)
>>  			return err;
>>  	}
>>  
>> -	if (output->edid)
>> -		edid = kmemdup(output->edid, sizeof(*edid), GFP_KERNEL);
>> +	if (output->drm_edid)
>> +		drm_edid = drm_edid_dup(output->drm_edid);
>>  	else if (output->ddc)
>> -		edid = drm_get_edid(connector, output->ddc);
>> +		drm_edid = drm_edid_read_ddc(connector, output->ddc);
>
> Annoyingly the dsi output is the only one not using
> drm_connector_init_with_ddc() despite that I think it could, so we can't
> yet use drm_edid_read here ...
>
>>  
>> -	cec_notifier_set_phys_addr_from_edid(output->cec, edid);
>> -	drm_connector_update_edid_property(connector, edid);
>> +	drm_edid_connector_update(connector, drm_edid);
>> +	cec_notifier_set_phys_addr(output->cec,
>> +				   connector->display_info.source_physical_address);
>>  
>> -	if (edid) {
>> -		err = drm_add_edid_modes(connector, edid);
>> -		kfree(edid);
>> -	}
>> +	err = drm_edid_connector_add_modes(connector);
>> +	drm_edid_free(drm_edid);
>>  
>>  	return err;
>>  }
>> @@ -98,6 +97,7 @@ static irqreturn_t hpd_irq(int irq, void *data)
>>  int tegra_output_probe(struct tegra_output *output)
>>  {
>>  	struct device_node *ddc, *panel;
>> +	const void *edid;
>>  	unsigned long flags;
>>  	int err, size;
>>  
>> @@ -124,8 +124,6 @@ int tegra_output_probe(struct tegra_output *output)
>>  			return PTR_ERR(output->panel);
>>  	}
>>  
>> -	output->edid = of_get_property(output->of_node, "nvidia,edid", &size);
>> -
>
> I think you can/should also delete tegra_output.edid completely, since not
> doing that freaked me out a bit until I checked everything :-) At least if
> I didn't git grep the wrong regex.

I thought I am removing tegra_output.edid. What am I missing?

>
> With that bikeshed addressed:
>
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Thanks,
Jani.


>
>>  	ddc = of_parse_phandle(output->of_node, "nvidia,ddc-i2c-bus", 0);
>>  	if (ddc) {
>>  		output->ddc = of_get_i2c_adapter_by_node(ddc);
>> @@ -137,6 +135,9 @@ int tegra_output_probe(struct tegra_output *output)
>>  		}
>>  	}
>>  
>> +	edid = of_get_property(output->of_node, "nvidia,edid", &size);
>> +	output->drm_edid = drm_edid_alloc(edid, size);
>> +
>>  	output->hpd_gpio = devm_fwnode_gpiod_get(output->dev,
>>  					of_fwnode_handle(output->of_node),
>>  					"nvidia,hpd",
>> @@ -187,6 +188,8 @@ int tegra_output_probe(struct tegra_output *output)
>>  	if (output->ddc)
>>  		i2c_put_adapter(output->ddc);
>>  
>> +	drm_edid_free(output->drm_edid);
>> +
>>  	return err;
>>  }
>>  
>> @@ -197,6 +200,8 @@ void tegra_output_remove(struct tegra_output *output)
>>  
>>  	if (output->ddc)
>>  		i2c_put_adapter(output->ddc);
>> +
>> +	drm_edid_free(output->drm_edid);
>>  }
>>  
>>  int tegra_output_init(struct drm_device *drm, struct tegra_output *output)
>> -- 
>> 2.39.2
>> 

-- 
Jani Nikula, Intel

Re: [PATCH 09/11] drm/tegra: convert to struct drm_edid

[Daniel Vetter]

On Thu, Aug 22, 2024 at 07:32:15PM +0300, Jani Nikula wrote:
> On Mon, 24 Jun 2024, Daniel Vetter <daniel@ffwll.ch> wrote:
> > On Tue, May 14, 2024 at 03:55:15PM +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: Thierry Reding <thierry.reding@gmail.com>
> >> Cc: Mikko Perttunen <mperttunen@nvidia.com>
> >> Cc: Jonathan Hunter <jonathanh@nvidia.com>
> >> Cc: linux-tegra@vger.kernel.org
> >> ---
> >>  drivers/gpu/drm/tegra/drm.h    |  2 +-
> >>  drivers/gpu/drm/tegra/output.c | 29 +++++++++++++++++------------
> >>  2 files changed, 18 insertions(+), 13 deletions(-)
> >> 
> >> diff --git a/drivers/gpu/drm/tegra/drm.h b/drivers/gpu/drm/tegra/drm.h
> >> index 682011166a8f..2f3781e04b0a 100644
> >> --- a/drivers/gpu/drm/tegra/drm.h
> >> +++ b/drivers/gpu/drm/tegra/drm.h
> >> @@ -133,7 +133,7 @@ struct tegra_output {
> >>  	struct drm_bridge *bridge;
> >>  	struct drm_panel *panel;
> >>  	struct i2c_adapter *ddc;
> >> -	const struct edid *edid;
> >> +	const struct drm_edid *drm_edid;
> 
> I keep reviving old stuff, sorry.
> 
> Please take note here, removing the edid member.
> 
> >>  	struct cec_notifier *cec;
> >>  	unsigned int hpd_irq;
> >>  	struct gpio_desc *hpd_gpio;
> >> diff --git a/drivers/gpu/drm/tegra/output.c b/drivers/gpu/drm/tegra/output.c
> >> index 4da3c3d1abbc..e6b5863fec71 100644
> >> --- a/drivers/gpu/drm/tegra/output.c
> >> +++ b/drivers/gpu/drm/tegra/output.c
> >> @@ -21,7 +21,7 @@
> >>  int tegra_output_connector_get_modes(struct drm_connector *connector)
> >>  {
> >>  	struct tegra_output *output = connector_to_output(connector);
> >> -	struct edid *edid = NULL;
> >> +	const struct drm_edid *drm_edid;
> >>  	int err = 0;
> >>  
> >>  	/*
> >> @@ -34,18 +34,17 @@ int tegra_output_connector_get_modes(struct drm_connector *connector)
> >>  			return err;
> >>  	}
> >>  
> >> -	if (output->edid)
> >> -		edid = kmemdup(output->edid, sizeof(*edid), GFP_KERNEL);
> >> +	if (output->drm_edid)
> >> +		drm_edid = drm_edid_dup(output->drm_edid);
> >>  	else if (output->ddc)
> >> -		edid = drm_get_edid(connector, output->ddc);
> >> +		drm_edid = drm_edid_read_ddc(connector, output->ddc);
> >
> > Annoyingly the dsi output is the only one not using
> > drm_connector_init_with_ddc() despite that I think it could, so we can't
> > yet use drm_edid_read here ...
> >
> >>  
> >> -	cec_notifier_set_phys_addr_from_edid(output->cec, edid);
> >> -	drm_connector_update_edid_property(connector, edid);
> >> +	drm_edid_connector_update(connector, drm_edid);
> >> +	cec_notifier_set_phys_addr(output->cec,
> >> +				   connector->display_info.source_physical_address);
> >>  
> >> -	if (edid) {
> >> -		err = drm_add_edid_modes(connector, edid);
> >> -		kfree(edid);
> >> -	}
> >> +	err = drm_edid_connector_add_modes(connector);
> >> +	drm_edid_free(drm_edid);
> >>  
> >>  	return err;
> >>  }
> >> @@ -98,6 +97,7 @@ static irqreturn_t hpd_irq(int irq, void *data)
> >>  int tegra_output_probe(struct tegra_output *output)
> >>  {
> >>  	struct device_node *ddc, *panel;
> >> +	const void *edid;
> >>  	unsigned long flags;
> >>  	int err, size;
> >>  
> >> @@ -124,8 +124,6 @@ int tegra_output_probe(struct tegra_output *output)
> >>  			return PTR_ERR(output->panel);
> >>  	}
> >>  
> >> -	output->edid = of_get_property(output->of_node, "nvidia,edid", &size);
> >> -
> >
> > I think you can/should also delete tegra_output.edid completely, since not
> > doing that freaked me out a bit until I checked everything :-) At least if
> > I didn't git grep the wrong regex.
> 
> I thought I am removing tegra_output.edid. What am I missing?

I'm sometimes just blind. rb without bikeshed applies, sorry for the
confusion.
-Sima

> 
> >
> > With that bikeshed addressed:
> >
> > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> Thanks,
> Jani.
> 
> 
> >
> >>  	ddc = of_parse_phandle(output->of_node, "nvidia,ddc-i2c-bus", 0);
> >>  	if (ddc) {
> >>  		output->ddc = of_get_i2c_adapter_by_node(ddc);
> >> @@ -137,6 +135,9 @@ int tegra_output_probe(struct tegra_output *output)
> >>  		}
> >>  	}
> >>  
> >> +	edid = of_get_property(output->of_node, "nvidia,edid", &size);
> >> +	output->drm_edid = drm_edid_alloc(edid, size);
> >> +
> >>  	output->hpd_gpio = devm_fwnode_gpiod_get(output->dev,
> >>  					of_fwnode_handle(output->of_node),
> >>  					"nvidia,hpd",
> >> @@ -187,6 +188,8 @@ int tegra_output_probe(struct tegra_output *output)
> >>  	if (output->ddc)
> >>  		i2c_put_adapter(output->ddc);
> >>  
> >> +	drm_edid_free(output->drm_edid);
> >> +
> >>  	return err;
> >>  }
> >>  
> >> @@ -197,6 +200,8 @@ void tegra_output_remove(struct tegra_output *output)
> >>  
> >>  	if (output->ddc)
> >>  		i2c_put_adapter(output->ddc);
> >> +
> >> +	drm_edid_free(output->drm_edid);
> >>  }
> >>  
> >>  int tegra_output_init(struct drm_device *drm, struct tegra_output *output)
> >> -- 
> >> 2.39.2
> >> 
> 
> -- 
> Jani Nikula, Intel

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

[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

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

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

[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

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

Re: [PATCH 00/11] drm: conversions to struct drm_edid

[Dmitry Baryshkov]

On Tue, 14 May 2024 15:55:06 +0300, Jani Nikula wrote:
> Convert more drivers to struct drm_edid.
> 
> Compile tested only.
> 
> Jani Nikula (11):
>   drm/rockchip: cdn-dp: get rid of drm_edid_raw()
>   drm/sti/sti_hdmi: convert to struct drm_edid
>   drm/bridge: analogix_dp: convert to struct drm_edid
>   drm/exynos: hdmi: convert to struct drm_edid
>   drm/hisilicon/hibmc: convert to struct drm_edid
>   drm/loongson/7a1000: convert to struct drm_edid
>   drm/loongson/7a2000: convert to struct drm_edid
>   drm/msm/dp: switch to struct drm_edid
>   drm/tegra: convert to struct drm_edid
>   drm/imx/tve: convert to struct drm_edid
>   drm/imx/ldb: convert to struct drm_edid
> 
> [...]

Applied, thanks!

[08/11] drm/msm/dp: switch to struct drm_edid
        https://gitlab.freedesktop.org/lumag/msm/-/commit/b1510bee17f5

Best regards,
-- 
Dmitry Baryshkov <dmitry.baryshkov@linaro.org>