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

[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