| Message ID | dd1c232cb85d5e0815af73c918953fa3b852baa2.1715691257.git.jani.nikula@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm: conversions to struct drm_edid | expand |
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 >
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 >>
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
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:
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(-)