| Message ID | 20231114105815.4188901-1-jani.nikula@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/probe-helper: convert drm_connector_helper_get_modes_from_ddc() to struct drm_edid | expand |
Hi Am 14.11.23 um 11:58 schrieb Jani Nikula: > Going forward, the struct drm_edid based functions drm_edid_read(), > drm_edid_connector_update() and drm_edid_connector_add_modes() are the > preferred ways of retrieving the EDID and updating the connector. > > Cc: Thomas Zimmermann <tzimmermann@suse.de> > Signed-off-by: Jani Nikula <jani.nikula@intel.com> Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de> If I'm not mistaken, the correct pattern is to read the EDID block in the detect callback and only parse it for modes in get_modes. If so, you might also inline this helper into its only caller in mgag200. I'll later split it up into detect and get_modes. Best regards Thomas > --- > drivers/gpu/drm/drm_probe_helper.c | 17 ++++++++--------- > 1 file changed, 8 insertions(+), 9 deletions(-) > > diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c > index 3f479483d7d8..309d88f13648 100644 > --- a/drivers/gpu/drm/drm_probe_helper.c > +++ b/drivers/gpu/drm/drm_probe_helper.c > @@ -1116,21 +1116,20 @@ EXPORT_SYMBOL(drm_crtc_helper_mode_valid_fixed); > */ > int drm_connector_helper_get_modes_from_ddc(struct drm_connector *connector) > { > - struct edid *edid; > - int count = 0; > + const struct drm_edid *drm_edid; > + int count; > > if (!connector->ddc) > return 0; > > - edid = drm_get_edid(connector, connector->ddc); > + drm_edid = drm_edid_read(connector); > + > + /* clears property if EDID is NULL */ > + drm_edid_connector_update(connector, drm_edid); > > - // clears property if EDID is NULL > - drm_connector_update_edid_property(connector, edid); > + count = drm_edid_connector_add_modes(connector); > > - if (edid) { > - count = drm_add_edid_modes(connector, edid); > - kfree(edid); > - } > + drm_edid_free(drm_edid); > > return count; > }
On Tue, 14 Nov 2023, Thomas Zimmermann <tzimmermann@suse.de> wrote: > Hi > > Am 14.11.23 um 11:58 schrieb Jani Nikula: >> Going forward, the struct drm_edid based functions drm_edid_read(), >> drm_edid_connector_update() and drm_edid_connector_add_modes() are the >> preferred ways of retrieving the EDID and updating the connector. >> >> Cc: Thomas Zimmermann <tzimmermann@suse.de> >> Signed-off-by: Jani Nikula <jani.nikula@intel.com> > > Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de> > > If I'm not mistaken, the correct pattern is to read the EDID block in > the detect callback and only parse it for modes in get_modes. If so, you > might also inline this helper into its only caller in mgag200. I'll > later split it up into detect and get_modes. That's what we do in i915 anyway. drm_edid_read() and drm_edid_connector_update() in ->detect and ->force, and drm_edid_connector_add_modes() in ->get_modes. BR, Jani. > > Best regards > Thomas > >> --- >> drivers/gpu/drm/drm_probe_helper.c | 17 ++++++++--------- >> 1 file changed, 8 insertions(+), 9 deletions(-) >> >> diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c >> index 3f479483d7d8..309d88f13648 100644 >> --- a/drivers/gpu/drm/drm_probe_helper.c >> +++ b/drivers/gpu/drm/drm_probe_helper.c >> @@ -1116,21 +1116,20 @@ EXPORT_SYMBOL(drm_crtc_helper_mode_valid_fixed); >> */ >> int drm_connector_helper_get_modes_from_ddc(struct drm_connector *connector) >> { >> - struct edid *edid; >> - int count = 0; >> + const struct drm_edid *drm_edid; >> + int count; >> >> if (!connector->ddc) >> return 0; >> >> - edid = drm_get_edid(connector, connector->ddc); >> + drm_edid = drm_edid_read(connector); >> + >> + /* clears property if EDID is NULL */ >> + drm_edid_connector_update(connector, drm_edid); >> >> - // clears property if EDID is NULL >> - drm_connector_update_edid_property(connector, edid); >> + count = drm_edid_connector_add_modes(connector); >> >> - if (edid) { >> - count = drm_add_edid_modes(connector, edid); >> - kfree(edid); >> - } >> + drm_edid_free(drm_edid); >> >> return count; >> }
On Tue, 14 Nov 2023, Thomas Zimmermann <tzimmermann@suse.de> wrote: > If I'm not mistaken, the correct pattern is to read the EDID block in > the detect callback and only parse it for modes in get_modes. If so, you > might also inline this helper into its only caller in mgag200. I'll > later split it up into detect and get_modes. Never merged this, so I sent your suggested alternative instead [1]. BR, Jani. [1] https://patchwork.freedesktop.org/series/128269/
diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c index 3f479483d7d8..309d88f13648 100644 --- a/drivers/gpu/drm/drm_probe_helper.c +++ b/drivers/gpu/drm/drm_probe_helper.c @@ -1116,21 +1116,20 @@ EXPORT_SYMBOL(drm_crtc_helper_mode_valid_fixed); */ int drm_connector_helper_get_modes_from_ddc(struct drm_connector *connector) { - struct edid *edid; - int count = 0; + const struct drm_edid *drm_edid; + int count; if (!connector->ddc) return 0; - edid = drm_get_edid(connector, connector->ddc); + drm_edid = drm_edid_read(connector); + + /* clears property if EDID is NULL */ + drm_edid_connector_update(connector, drm_edid); - // clears property if EDID is NULL - drm_connector_update_edid_property(connector, edid); + count = drm_edid_connector_add_modes(connector); - if (edid) { - count = drm_add_edid_modes(connector, edid); - kfree(edid); - } + drm_edid_free(drm_edid); return count; }
Going forward, the struct drm_edid based functions drm_edid_read(), drm_edid_connector_update() and drm_edid_connector_add_modes() are the preferred ways of retrieving the EDID and updating the connector. Cc: Thomas Zimmermann <tzimmermann@suse.de> Signed-off-by: Jani Nikula <jani.nikula@intel.com> --- drivers/gpu/drm/drm_probe_helper.c | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-)