[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 at 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 at intel.com>
> >> 
> >> ---
> >> 
> >> Cc: Alain Volmat <alain.volmat at foss.st.com>
> >> Cc: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
> >> Cc: Maxime Ripard <mripard at kernel.org>
> >> Cc: Thomas Zimmermann <tzimmermann at 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 at 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 at ffwll.ch>
> >
> >>  
> >>  fail:
> >> -- 
> >> 2.39.2
> >> 
> 
> -- 
> Jani Nikula, Intel

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