| Message ID | 20240911180650.820598-1-tejasvipin76@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] drm/gma500: replace drm_detect_hdmi_monitor() with drm_display_info.is_hdmi | expand |
Hi Am 11.09.24 um 20:06 schrieb Tejas Vipin: > Replace drm_detect_hdmi_monitor() with drm_display_info.is_hdmi since > monitor HDMI information is available after EDID is parsed. Additionally > rewrite the code the code to have fewer indentation levels. The problem is that the entire logic is outdated. The content of cdv_hdmi_detect() should go into cdv_hdmi_get_modes(), the detect_ctx callback should be set to drm_connector_helper_detect_from_ddc() and cdv_hdmi_detect() should be deleted. The result is that ->detect_ctx will detect the presence of a display and ->get_modes will update EDID and other properties. Do you have a device for testing such a change? Best regards Thomas > > Signed-off-by: Tejas Vipin <tejasvipin76@gmail.com> > --- > Changes in v2: > - Use drm_edid instead of edid > > Link to v1: https://lore.kernel.org/all/20240910051856.700210-1-tejasvipin76@gmail.com/ > --- > drivers/gpu/drm/gma500/cdv_intel_hdmi.c | 24 +++++++++++++----------- > 1 file changed, 13 insertions(+), 11 deletions(-) > > diff --git a/drivers/gpu/drm/gma500/cdv_intel_hdmi.c b/drivers/gpu/drm/gma500/cdv_intel_hdmi.c > index 2d95e0471291..701f8bbd5f2b 100644 > --- a/drivers/gpu/drm/gma500/cdv_intel_hdmi.c > +++ b/drivers/gpu/drm/gma500/cdv_intel_hdmi.c > @@ -128,23 +128,25 @@ static enum drm_connector_status cdv_hdmi_detect( > { > struct gma_encoder *gma_encoder = gma_attached_encoder(connector); > struct mid_intel_hdmi_priv *hdmi_priv = gma_encoder->dev_priv; > - struct edid *edid = NULL; > + const struct drm_edid *drm_edid; > + int ret; > enum drm_connector_status status = connector_status_disconnected; > > - edid = drm_get_edid(connector, connector->ddc); > + drm_edid = drm_edid_read_ddc(connector, connector->ddc); > + ret = drm_edid_connector_update(connector, drm_edid); > > hdmi_priv->has_hdmi_sink = false; > hdmi_priv->has_hdmi_audio = false; > - if (edid) { > - if (edid->input & DRM_EDID_INPUT_DIGITAL) { > - status = connector_status_connected; > - hdmi_priv->has_hdmi_sink = > - drm_detect_hdmi_monitor(edid); > - hdmi_priv->has_hdmi_audio = > - drm_detect_monitor_audio(edid); > - } > - kfree(edid); > + if (ret) > + return status; > + > + if (drm_edid_is_digital(drm_edid)) { > + status = connector_status_connected; > + hdmi_priv->has_hdmi_sink = connector->display_info.is_hdmi; > + hdmi_priv->has_hdmi_audio = connector->display_info.has_audio; > } > + drm_edid_free(drm_edid); > + > return status; > } >
On Thu, 12 Sep 2024, Thomas Zimmermann <tzimmermann@suse.de> wrote: > Hi > > Am 11.09.24 um 20:06 schrieb Tejas Vipin: >> Replace drm_detect_hdmi_monitor() with drm_display_info.is_hdmi since >> monitor HDMI information is available after EDID is parsed. Additionally >> rewrite the code the code to have fewer indentation levels. > > The problem is that the entire logic is outdated. The content > of cdv_hdmi_detect() should go into cdv_hdmi_get_modes(), the detect_ctx > callback should be set to drm_connector_helper_detect_from_ddc() and > cdv_hdmi_detect() should be deleted. The result is that ->detect_ctx > will detect the presence of a display and ->get_modes will update EDID > and other properties. I guess I didn't get the memo on this one. What's the problem with reading the EDID at detect? The subsequent drm_edid_connector_add_modes() called from .get_modes() does not need to read the EDID again. I think it should be fine to do incremental refactors like the patch at hand (modulo some issues I mention below). BR, Jani. > > Do you have a device for testing such a change? > > Best regards > Thomas > >> >> Signed-off-by: Tejas Vipin <tejasvipin76@gmail.com> >> --- >> Changes in v2: >> - Use drm_edid instead of edid >> >> Link to v1: https://lore.kernel.org/all/20240910051856.700210-1-tejasvipin76@gmail.com/ >> --- >> drivers/gpu/drm/gma500/cdv_intel_hdmi.c | 24 +++++++++++++----------- >> 1 file changed, 13 insertions(+), 11 deletions(-) >> >> diff --git a/drivers/gpu/drm/gma500/cdv_intel_hdmi.c b/drivers/gpu/drm/gma500/cdv_intel_hdmi.c >> index 2d95e0471291..701f8bbd5f2b 100644 >> --- a/drivers/gpu/drm/gma500/cdv_intel_hdmi.c >> +++ b/drivers/gpu/drm/gma500/cdv_intel_hdmi.c >> @@ -128,23 +128,25 @@ static enum drm_connector_status cdv_hdmi_detect( >> { >> struct gma_encoder *gma_encoder = gma_attached_encoder(connector); >> struct mid_intel_hdmi_priv *hdmi_priv = gma_encoder->dev_priv; >> - struct edid *edid = NULL; >> + const struct drm_edid *drm_edid; >> + int ret; >> enum drm_connector_status status = connector_status_disconnected; >> >> - edid = drm_get_edid(connector, connector->ddc); >> + drm_edid = drm_edid_read_ddc(connector, connector->ddc); Just drm_edid_read() is enough when you're using connector->ddc. >> + ret = drm_edid_connector_update(connector, drm_edid); >> >> hdmi_priv->has_hdmi_sink = false; >> hdmi_priv->has_hdmi_audio = false; >> - if (edid) { >> - if (edid->input & DRM_EDID_INPUT_DIGITAL) { >> - status = connector_status_connected; >> - hdmi_priv->has_hdmi_sink = >> - drm_detect_hdmi_monitor(edid); >> - hdmi_priv->has_hdmi_audio = >> - drm_detect_monitor_audio(edid); >> - } >> - kfree(edid); >> + if (ret) This error path leaks the EDID. >> + return status; >> + >> + if (drm_edid_is_digital(drm_edid)) { >> + status = connector_status_connected; >> + hdmi_priv->has_hdmi_sink = connector->display_info.is_hdmi; >> + hdmi_priv->has_hdmi_audio = connector->display_info.has_audio; >> } >> + drm_edid_free(drm_edid); >> + >> return status; >> } >>
On Thu, 12 Sep 2024, Jani Nikula <jani.nikula@linux.intel.com> wrote: > On Thu, 12 Sep 2024, Thomas Zimmermann <tzimmermann@suse.de> wrote: >> Hi >> >> Am 11.09.24 um 20:06 schrieb Tejas Vipin: >>> Replace drm_detect_hdmi_monitor() with drm_display_info.is_hdmi since >>> monitor HDMI information is available after EDID is parsed. Additionally >>> rewrite the code the code to have fewer indentation levels. >> >> The problem is that the entire logic is outdated. The content >> of cdv_hdmi_detect() should go into cdv_hdmi_get_modes(), the detect_ctx >> callback should be set to drm_connector_helper_detect_from_ddc() and >> cdv_hdmi_detect() should be deleted. The result is that ->detect_ctx >> will detect the presence of a display and ->get_modes will update EDID >> and other properties. > > I guess I didn't get the memo on this one. > > What's the problem with reading the EDID at detect? The subsequent > drm_edid_connector_add_modes() called from .get_modes() does not need to > read the EDID again. Moreover, in this case .detect() only detects digital displays as reported by EDID. If you postpone that to .get_modes(), the probe helper will still report connected, and invent non-EDID fallback modes. The behaviour changes. > I think it should be fine to do incremental refactors like the patch at > hand (modulo some issues I mention below). Another issue in the patch is that it should also change .get_modes() to not read the EDID again, but just call drm_edid_connector_add_modes(). BR, Jani. > > BR, > Jani. > > >> >> Do you have a device for testing such a change? >> >> Best regards >> Thomas >> >>> >>> Signed-off-by: Tejas Vipin <tejasvipin76@gmail.com> >>> --- >>> Changes in v2: >>> - Use drm_edid instead of edid >>> >>> Link to v1: https://lore.kernel.org/all/20240910051856.700210-1-tejasvipin76@gmail.com/ >>> --- >>> drivers/gpu/drm/gma500/cdv_intel_hdmi.c | 24 +++++++++++++----------- >>> 1 file changed, 13 insertions(+), 11 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/gma500/cdv_intel_hdmi.c b/drivers/gpu/drm/gma500/cdv_intel_hdmi.c >>> index 2d95e0471291..701f8bbd5f2b 100644 >>> --- a/drivers/gpu/drm/gma500/cdv_intel_hdmi.c >>> +++ b/drivers/gpu/drm/gma500/cdv_intel_hdmi.c >>> @@ -128,23 +128,25 @@ static enum drm_connector_status cdv_hdmi_detect( >>> { >>> struct gma_encoder *gma_encoder = gma_attached_encoder(connector); >>> struct mid_intel_hdmi_priv *hdmi_priv = gma_encoder->dev_priv; >>> - struct edid *edid = NULL; >>> + const struct drm_edid *drm_edid; >>> + int ret; >>> enum drm_connector_status status = connector_status_disconnected; >>> >>> - edid = drm_get_edid(connector, connector->ddc); >>> + drm_edid = drm_edid_read_ddc(connector, connector->ddc); > > Just drm_edid_read() is enough when you're using connector->ddc. > >>> + ret = drm_edid_connector_update(connector, drm_edid); >>> >>> hdmi_priv->has_hdmi_sink = false; >>> hdmi_priv->has_hdmi_audio = false; >>> - if (edid) { >>> - if (edid->input & DRM_EDID_INPUT_DIGITAL) { >>> - status = connector_status_connected; >>> - hdmi_priv->has_hdmi_sink = >>> - drm_detect_hdmi_monitor(edid); >>> - hdmi_priv->has_hdmi_audio = >>> - drm_detect_monitor_audio(edid); >>> - } >>> - kfree(edid); >>> + if (ret) > > This error path leaks the EDID. > >>> + return status; >>> + >>> + if (drm_edid_is_digital(drm_edid)) { >>> + status = connector_status_connected; >>> + hdmi_priv->has_hdmi_sink = connector->display_info.is_hdmi; >>> + hdmi_priv->has_hdmi_audio = connector->display_info.has_audio; >>> } >>> + drm_edid_free(drm_edid); >>> + >>> return status; >>> } >>>
Hi Am 12.09.24 um 10:48 schrieb Jani Nikula: > On Thu, 12 Sep 2024, Thomas Zimmermann <tzimmermann@suse.de> wrote: >> Hi >> >> Am 11.09.24 um 20:06 schrieb Tejas Vipin: >>> Replace drm_detect_hdmi_monitor() with drm_display_info.is_hdmi since >>> monitor HDMI information is available after EDID is parsed. Additionally >>> rewrite the code the code to have fewer indentation levels. >> The problem is that the entire logic is outdated. The content >> of cdv_hdmi_detect() should go into cdv_hdmi_get_modes(), the detect_ctx >> callback should be set to drm_connector_helper_detect_from_ddc() and >> cdv_hdmi_detect() should be deleted. The result is that ->detect_ctx >> will detect the presence of a display and ->get_modes will update EDID >> and other properties. > I guess I didn't get the memo on this one. > > What's the problem with reading the EDID at detect? The subsequent > drm_edid_connector_add_modes() called from .get_modes() does not need to > read the EDID again. With drm_connector_helper_detect_from_ddc() there is already a helper for detection. It makes sense to use it. And if we continue to update the properties in detect (instead of get_modes), what is the correct connector_status on errors? Right now and with the patch applied, detect returns status_disconnected on errors. But this isn't correct if there actually is a display. By separating detect and get_modes cleanly, we can detect the display reliably, but also handle errors better than we currently do in gma500. Get_modes is already expected to update the EDID property, [1] for detect it's not so clear AFAICT. I think that from a design perspective, it makes sense to have a read-only function that only detects the physical state of the connector and a read-write function that updates the connector's properties. Best regards Thomas [1] https://elixir.bootlin.com/linux/v6.10.9/source/include/drm/drm_modeset_helper_vtables.h#L865 > > I think it should be fine to do incremental refactors like the patch at > hand (modulo some issues I mention below). > > BR, > Jani. > > >> Do you have a device for testing such a change? >> >> Best regards >> Thomas >> >>> Signed-off-by: Tejas Vipin <tejasvipin76@gmail.com> >>> --- >>> Changes in v2: >>> - Use drm_edid instead of edid >>> >>> Link to v1: https://lore.kernel.org/all/20240910051856.700210-1-tejasvipin76@gmail.com/ >>> --- >>> drivers/gpu/drm/gma500/cdv_intel_hdmi.c | 24 +++++++++++++----------- >>> 1 file changed, 13 insertions(+), 11 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/gma500/cdv_intel_hdmi.c b/drivers/gpu/drm/gma500/cdv_intel_hdmi.c >>> index 2d95e0471291..701f8bbd5f2b 100644 >>> --- a/drivers/gpu/drm/gma500/cdv_intel_hdmi.c >>> +++ b/drivers/gpu/drm/gma500/cdv_intel_hdmi.c >>> @@ -128,23 +128,25 @@ static enum drm_connector_status cdv_hdmi_detect( >>> { >>> struct gma_encoder *gma_encoder = gma_attached_encoder(connector); >>> struct mid_intel_hdmi_priv *hdmi_priv = gma_encoder->dev_priv; >>> - struct edid *edid = NULL; >>> + const struct drm_edid *drm_edid; >>> + int ret; >>> enum drm_connector_status status = connector_status_disconnected; >>> >>> - edid = drm_get_edid(connector, connector->ddc); >>> + drm_edid = drm_edid_read_ddc(connector, connector->ddc); > Just drm_edid_read() is enough when you're using connector->ddc. > >>> + ret = drm_edid_connector_update(connector, drm_edid); >>> >>> hdmi_priv->has_hdmi_sink = false; >>> hdmi_priv->has_hdmi_audio = false; >>> - if (edid) { >>> - if (edid->input & DRM_EDID_INPUT_DIGITAL) { >>> - status = connector_status_connected; >>> - hdmi_priv->has_hdmi_sink = >>> - drm_detect_hdmi_monitor(edid); >>> - hdmi_priv->has_hdmi_audio = >>> - drm_detect_monitor_audio(edid); >>> - } >>> - kfree(edid); >>> + if (ret) > This error path leaks the EDID. > >>> + return status; >>> + >>> + if (drm_edid_is_digital(drm_edid)) { >>> + status = connector_status_connected; >>> + hdmi_priv->has_hdmi_sink = connector->display_info.is_hdmi; >>> + hdmi_priv->has_hdmi_audio = connector->display_info.has_audio; >>> } >>> + drm_edid_free(drm_edid); >>> + >>> return status; >>> } >>>
Hi Am 12.09.24 um 10:56 schrieb Jani Nikula: > On Thu, 12 Sep 2024, Jani Nikula <jani.nikula@linux.intel.com> wrote: >> On Thu, 12 Sep 2024, Thomas Zimmermann <tzimmermann@suse.de> wrote: >>> Hi >>> >>> Am 11.09.24 um 20:06 schrieb Tejas Vipin: >>>> Replace drm_detect_hdmi_monitor() with drm_display_info.is_hdmi since >>>> monitor HDMI information is available after EDID is parsed. Additionally >>>> rewrite the code the code to have fewer indentation levels. >>> The problem is that the entire logic is outdated. The content >>> of cdv_hdmi_detect() should go into cdv_hdmi_get_modes(), the detect_ctx >>> callback should be set to drm_connector_helper_detect_from_ddc() and >>> cdv_hdmi_detect() should be deleted. The result is that ->detect_ctx >>> will detect the presence of a display and ->get_modes will update EDID >>> and other properties. >> I guess I didn't get the memo on this one. >> >> What's the problem with reading the EDID at detect? The subsequent >> drm_edid_connector_add_modes() called from .get_modes() does not need to >> read the EDID again. > Moreover, in this case .detect() only detects digital displays as > reported by EDID. If you postpone that to .get_modes(), the probe helper > will still report connected, and invent non-EDID fallback modes. The > behaviour changes. The change in behavior is intentional, because the current test seems arbitrary. Does the driver not work with analog outputs? Best regards Thomas > >> I think it should be fine to do incremental refactors like the patch at >> hand (modulo some issues I mention below). > Another issue in the patch is that it should also change .get_modes() to > not read the EDID again, but just call drm_edid_connector_add_modes(). > > BR, > Jani. > >> BR, >> Jani. >> >> >>> Do you have a device for testing such a change? >>> >>> Best regards >>> Thomas >>> >>>> Signed-off-by: Tejas Vipin <tejasvipin76@gmail.com> >>>> --- >>>> Changes in v2: >>>> - Use drm_edid instead of edid >>>> >>>> Link to v1: https://lore.kernel.org/all/20240910051856.700210-1-tejasvipin76@gmail.com/ >>>> --- >>>> drivers/gpu/drm/gma500/cdv_intel_hdmi.c | 24 +++++++++++++----------- >>>> 1 file changed, 13 insertions(+), 11 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/gma500/cdv_intel_hdmi.c b/drivers/gpu/drm/gma500/cdv_intel_hdmi.c >>>> index 2d95e0471291..701f8bbd5f2b 100644 >>>> --- a/drivers/gpu/drm/gma500/cdv_intel_hdmi.c >>>> +++ b/drivers/gpu/drm/gma500/cdv_intel_hdmi.c >>>> @@ -128,23 +128,25 @@ static enum drm_connector_status cdv_hdmi_detect( >>>> { >>>> struct gma_encoder *gma_encoder = gma_attached_encoder(connector); >>>> struct mid_intel_hdmi_priv *hdmi_priv = gma_encoder->dev_priv; >>>> - struct edid *edid = NULL; >>>> + const struct drm_edid *drm_edid; >>>> + int ret; >>>> enum drm_connector_status status = connector_status_disconnected; >>>> >>>> - edid = drm_get_edid(connector, connector->ddc); >>>> + drm_edid = drm_edid_read_ddc(connector, connector->ddc); >> Just drm_edid_read() is enough when you're using connector->ddc. >> >>>> + ret = drm_edid_connector_update(connector, drm_edid); >>>> >>>> hdmi_priv->has_hdmi_sink = false; >>>> hdmi_priv->has_hdmi_audio = false; >>>> - if (edid) { >>>> - if (edid->input & DRM_EDID_INPUT_DIGITAL) { >>>> - status = connector_status_connected; >>>> - hdmi_priv->has_hdmi_sink = >>>> - drm_detect_hdmi_monitor(edid); >>>> - hdmi_priv->has_hdmi_audio = >>>> - drm_detect_monitor_audio(edid); >>>> - } >>>> - kfree(edid); >>>> + if (ret) >> This error path leaks the EDID. >> >>>> + return status; >>>> + >>>> + if (drm_edid_is_digital(drm_edid)) { >>>> + status = connector_status_connected; >>>> + hdmi_priv->has_hdmi_sink = connector->display_info.is_hdmi; >>>> + hdmi_priv->has_hdmi_audio = connector->display_info.has_audio; >>>> } >>>> + drm_edid_free(drm_edid); >>>> + >>>> return status; >>>> } >>>>
On Thu, 12 Sep 2024, Thomas Zimmermann <tzimmermann@suse.de> wrote: > Hi > > Am 12.09.24 um 10:48 schrieb Jani Nikula: >> On Thu, 12 Sep 2024, Thomas Zimmermann <tzimmermann@suse.de> wrote: >>> Hi >>> >>> Am 11.09.24 um 20:06 schrieb Tejas Vipin: >>>> Replace drm_detect_hdmi_monitor() with drm_display_info.is_hdmi since >>>> monitor HDMI information is available after EDID is parsed. Additionally >>>> rewrite the code the code to have fewer indentation levels. >>> The problem is that the entire logic is outdated. The content >>> of cdv_hdmi_detect() should go into cdv_hdmi_get_modes(), the detect_ctx >>> callback should be set to drm_connector_helper_detect_from_ddc() and >>> cdv_hdmi_detect() should be deleted. The result is that ->detect_ctx >>> will detect the presence of a display and ->get_modes will update EDID >>> and other properties. >> I guess I didn't get the memo on this one. >> >> What's the problem with reading the EDID at detect? The subsequent >> drm_edid_connector_add_modes() called from .get_modes() does not need to >> read the EDID again. > > With drm_connector_helper_detect_from_ddc() there is already a helper > for detection. It makes sense to use it. And if we continue to update > the properties in detect (instead of get_modes), what is the correct > connector_status on errors? Right now and with the patch applied, detect > returns status_disconnected on errors. But this isn't correct if there > actually is a display. By separating detect and get_modes cleanly, we > can detect the display reliably, but also handle errors better than we > currently do in gma500. Get_modes is already expected to update the EDID > property, [1] for detect it's not so clear AFAICT. I think that from a > design perspective, it makes sense to have a read-only function that > only detects the physical state of the connector and a read-write > function that updates the connector's properties. Best regards Thomas > [1] > https://elixir.bootlin.com/linux/v6.10.9/source/include/drm/drm_modeset_helper_vtables.h#L865 So what if you can probe DDC but can't actually read an EDID of any kind? IMO that's a detect failure. Or how about things like CEC attach? Seems natural to do it at .detect(). Doing it at .get_modes() just seems wrong. However, it needs the EDID for physical address. I just don't think one size fits all here. BR, Jani. > >> >> I think it should be fine to do incremental refactors like the patch at >> hand (modulo some issues I mention below). >> >> BR, >> Jani. >> >> >>> Do you have a device for testing such a change? >>> >>> Best regards >>> Thomas >>> >>>> Signed-off-by: Tejas Vipin <tejasvipin76@gmail.com> >>>> --- >>>> Changes in v2: >>>> - Use drm_edid instead of edid >>>> >>>> Link to v1: https://lore.kernel.org/all/20240910051856.700210-1-tejasvipin76@gmail.com/ >>>> --- >>>> drivers/gpu/drm/gma500/cdv_intel_hdmi.c | 24 +++++++++++++----------- >>>> 1 file changed, 13 insertions(+), 11 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/gma500/cdv_intel_hdmi.c b/drivers/gpu/drm/gma500/cdv_intel_hdmi.c >>>> index 2d95e0471291..701f8bbd5f2b 100644 >>>> --- a/drivers/gpu/drm/gma500/cdv_intel_hdmi.c >>>> +++ b/drivers/gpu/drm/gma500/cdv_intel_hdmi.c >>>> @@ -128,23 +128,25 @@ static enum drm_connector_status cdv_hdmi_detect( >>>> { >>>> struct gma_encoder *gma_encoder = gma_attached_encoder(connector); >>>> struct mid_intel_hdmi_priv *hdmi_priv = gma_encoder->dev_priv; >>>> - struct edid *edid = NULL; >>>> + const struct drm_edid *drm_edid; >>>> + int ret; >>>> enum drm_connector_status status = connector_status_disconnected; >>>> >>>> - edid = drm_get_edid(connector, connector->ddc); >>>> + drm_edid = drm_edid_read_ddc(connector, connector->ddc); >> Just drm_edid_read() is enough when you're using connector->ddc. >> >>>> + ret = drm_edid_connector_update(connector, drm_edid); >>>> >>>> hdmi_priv->has_hdmi_sink = false; >>>> hdmi_priv->has_hdmi_audio = false; >>>> - if (edid) { >>>> - if (edid->input & DRM_EDID_INPUT_DIGITAL) { >>>> - status = connector_status_connected; >>>> - hdmi_priv->has_hdmi_sink = >>>> - drm_detect_hdmi_monitor(edid); >>>> - hdmi_priv->has_hdmi_audio = >>>> - drm_detect_monitor_audio(edid); >>>> - } >>>> - kfree(edid); >>>> + if (ret) >> This error path leaks the EDID. >> >>>> + return status; >>>> + >>>> + if (drm_edid_is_digital(drm_edid)) { >>>> + status = connector_status_connected; >>>> + hdmi_priv->has_hdmi_sink = connector->display_info.is_hdmi; >>>> + hdmi_priv->has_hdmi_audio = connector->display_info.has_audio; >>>> } >>>> + drm_edid_free(drm_edid); >>>> + >>>> return status; >>>> } >>>>
On Thu, 12 Sep 2024, Thomas Zimmermann <tzimmermann@suse.de> wrote: > Am 12.09.24 um 10:56 schrieb Jani Nikula: >> Moreover, in this case .detect() only detects digital displays as >> reported by EDID. If you postpone that to .get_modes(), the probe helper >> will still report connected, and invent non-EDID fallback modes. The >> behaviour changes. > > The change in behavior is intentional, because the current test seems > arbitrary. Does the driver not work with analog outputs? Not on a DVI/HDMI port. Same with i915. That's possibly the only way to distinguish a DVI-A display connected to DVI-D source. BR, Jani.
Hi Am 12.09.24 um 11:38 schrieb Jani Nikula: > On Thu, 12 Sep 2024, Thomas Zimmermann <tzimmermann@suse.de> wrote: >> Am 12.09.24 um 10:56 schrieb Jani Nikula: >>> Moreover, in this case .detect() only detects digital displays as >>> reported by EDID. If you postpone that to .get_modes(), the probe helper >>> will still report connected, and invent non-EDID fallback modes. The >>> behaviour changes. >> The change in behavior is intentional, because the current test seems >> arbitrary. Does the driver not work with analog outputs? > Not on a DVI/HDMI port. Same with i915. > > That's possibly the only way to distinguish a DVI-A display connected to > DVI-D source. That's a detect failure, but IMHO our probe helpers should really handle this case. Best regards Thomas > > > BR, > Jani. > >
Hi Am 12.09.24 um 11:30 schrieb Jani Nikula: > On Thu, 12 Sep 2024, Thomas Zimmermann <tzimmermann@suse.de> wrote: >> Hi >> >> Am 12.09.24 um 10:48 schrieb Jani Nikula: >>> On Thu, 12 Sep 2024, Thomas Zimmermann <tzimmermann@suse.de> wrote: >>>> Hi >>>> >>>> Am 11.09.24 um 20:06 schrieb Tejas Vipin: >>>>> Replace drm_detect_hdmi_monitor() with drm_display_info.is_hdmi since >>>>> monitor HDMI information is available after EDID is parsed. Additionally >>>>> rewrite the code the code to have fewer indentation levels. >>>> The problem is that the entire logic is outdated. The content >>>> of cdv_hdmi_detect() should go into cdv_hdmi_get_modes(), the detect_ctx >>>> callback should be set to drm_connector_helper_detect_from_ddc() and >>>> cdv_hdmi_detect() should be deleted. The result is that ->detect_ctx >>>> will detect the presence of a display and ->get_modes will update EDID >>>> and other properties. >>> I guess I didn't get the memo on this one. >>> >>> What's the problem with reading the EDID at detect? The subsequent >>> drm_edid_connector_add_modes() called from .get_modes() does not need to >>> read the EDID again. >> With drm_connector_helper_detect_from_ddc() there is already a helper >> for detection. It makes sense to use it. And if we continue to update >> the properties in detect (instead of get_modes), what is the correct >> connector_status on errors? Right now and with the patch applied, detect >> returns status_disconnected on errors. But this isn't correct if there >> actually is a display. By separating detect and get_modes cleanly, we >> can detect the display reliably, but also handle errors better than we >> currently do in gma500. Get_modes is already expected to update the EDID >> property, [1] for detect it's not so clear AFAICT. I think that from a >> design perspective, it makes sense to have a read-only function that >> only detects the physical state of the connector and a read-write >> function that updates the connector's properties. Best regards Thomas >> [1] >> https://elixir.bootlin.com/linux/v6.10.9/source/include/drm/drm_modeset_helper_vtables.h#L865 > So what if you can probe DDC but can't actually read an EDID of any > kind? IMO that's a detect failure. Not being able to read the EDID is not a failure IMHO. It's better to report a detected display and only provide minimal support, than to outright reject it. The display is essential for most users being able to use the computer at all, so it's often better to display something at lower quality than display nothing at all. > > Or how about things like CEC attach? Seems natural to do it at > .detect(). Doing it at .get_modes() just seems wrong. However, it needs > the EDID for physical address. > > I just don't think one size fits all here. The good thing about the EDID probe helpers is that it only reads a minimal amount of data, like a single byte or the EDID header, so something like that. Many drivers poll the DDC every 10 seconds via ->detect. Which means that code running in ->detect possibly runs concurrently to other DRM operations, such as page flips. Hence, ->detect can interfere with the driver's hot path. The call to ->get_modes usually only runs after the connector status changes to connected, which rarely happens. It's also not time critical as no modeset has happened yet. And as everyone copies code from everyone else, we should establish best practices that take such things into account. Even with drivers that don't use connector polling, such as gma500, we should encourage devs to do the right thing and move code out of ->detect. Best regards Thomas > > BR, > Jani. > > >>> I think it should be fine to do incremental refactors like the patch at >>> hand (modulo some issues I mention below). >>> >>> BR, >>> Jani. >>> >>> >>>> Do you have a device for testing such a change? >>>> >>>> Best regards >>>> Thomas >>>> >>>>> Signed-off-by: Tejas Vipin <tejasvipin76@gmail.com> >>>>> --- >>>>> Changes in v2: >>>>> - Use drm_edid instead of edid >>>>> >>>>> Link to v1: https://lore.kernel.org/all/20240910051856.700210-1-tejasvipin76@gmail.com/ >>>>> --- >>>>> drivers/gpu/drm/gma500/cdv_intel_hdmi.c | 24 +++++++++++++----------- >>>>> 1 file changed, 13 insertions(+), 11 deletions(-) >>>>> >>>>> diff --git a/drivers/gpu/drm/gma500/cdv_intel_hdmi.c b/drivers/gpu/drm/gma500/cdv_intel_hdmi.c >>>>> index 2d95e0471291..701f8bbd5f2b 100644 >>>>> --- a/drivers/gpu/drm/gma500/cdv_intel_hdmi.c >>>>> +++ b/drivers/gpu/drm/gma500/cdv_intel_hdmi.c >>>>> @@ -128,23 +128,25 @@ static enum drm_connector_status cdv_hdmi_detect( >>>>> { >>>>> struct gma_encoder *gma_encoder = gma_attached_encoder(connector); >>>>> struct mid_intel_hdmi_priv *hdmi_priv = gma_encoder->dev_priv; >>>>> - struct edid *edid = NULL; >>>>> + const struct drm_edid *drm_edid; >>>>> + int ret; >>>>> enum drm_connector_status status = connector_status_disconnected; >>>>> >>>>> - edid = drm_get_edid(connector, connector->ddc); >>>>> + drm_edid = drm_edid_read_ddc(connector, connector->ddc); >>> Just drm_edid_read() is enough when you're using connector->ddc. >>> >>>>> + ret = drm_edid_connector_update(connector, drm_edid); >>>>> >>>>> hdmi_priv->has_hdmi_sink = false; >>>>> hdmi_priv->has_hdmi_audio = false; >>>>> - if (edid) { >>>>> - if (edid->input & DRM_EDID_INPUT_DIGITAL) { >>>>> - status = connector_status_connected; >>>>> - hdmi_priv->has_hdmi_sink = >>>>> - drm_detect_hdmi_monitor(edid); >>>>> - hdmi_priv->has_hdmi_audio = >>>>> - drm_detect_monitor_audio(edid); >>>>> - } >>>>> - kfree(edid); >>>>> + if (ret) >>> This error path leaks the EDID. >>> >>>>> + return status; >>>>> + >>>>> + if (drm_edid_is_digital(drm_edid)) { >>>>> + status = connector_status_connected; >>>>> + hdmi_priv->has_hdmi_sink = connector->display_info.is_hdmi; >>>>> + hdmi_priv->has_hdmi_audio = connector->display_info.has_audio; >>>>> } >>>>> + drm_edid_free(drm_edid); >>>>> + >>>>> return status; >>>>> } >>>>>
On Thu, 12 Sep 2024, Thomas Zimmermann <tzimmermann@suse.de> wrote: > Hi > > Am 12.09.24 um 11:38 schrieb Jani Nikula: >> On Thu, 12 Sep 2024, Thomas Zimmermann <tzimmermann@suse.de> wrote: >>> Am 12.09.24 um 10:56 schrieb Jani Nikula: >>>> Moreover, in this case .detect() only detects digital displays as >>>> reported by EDID. If you postpone that to .get_modes(), the probe helper >>>> will still report connected, and invent non-EDID fallback modes. The >>>> behaviour changes. >>> The change in behavior is intentional, because the current test seems >>> arbitrary. Does the driver not work with analog outputs? >> Not on a DVI/HDMI port. Same with i915. >> >> That's possibly the only way to distinguish a DVI-A display connected to >> DVI-D source. > > That's a detect failure, but IMHO our probe helpers should really handle > this case. How? Allow returning detect failures from .get_modes()? BR, Jani.
Hi Am 12.09.24 um 13:25 schrieb Jani Nikula: > On Thu, 12 Sep 2024, Thomas Zimmermann <tzimmermann@suse.de> wrote: >> Hi >> >> Am 12.09.24 um 11:38 schrieb Jani Nikula: >>> On Thu, 12 Sep 2024, Thomas Zimmermann <tzimmermann@suse.de> wrote: >>>> Am 12.09.24 um 10:56 schrieb Jani Nikula: >>>>> Moreover, in this case .detect() only detects digital displays as >>>>> reported by EDID. If you postpone that to .get_modes(), the probe helper >>>>> will still report connected, and invent non-EDID fallback modes. The >>>>> behaviour changes. >>>> The change in behavior is intentional, because the current test seems >>>> arbitrary. Does the driver not work with analog outputs? >>> Not on a DVI/HDMI port. Same with i915. >>> >>> That's possibly the only way to distinguish a DVI-A display connected to >>> DVI-D source. >> That's a detect failure, but IMHO our probe helpers should really handle >> this case. > How? Allow returning detect failures from .get_modes()? Something like that, I guess. For the specific problem it would be enough to read the first 20 bytes of EDID data on DVI connectors and test the digital-input flag bit against the exact connector requirements. drm_probe_ddc() could do this. Non-DVI connectors would continue to read a single bytes to detect the DDC. For more sophisticated problems, it would be good to introduce an intermediate callback that updates the connector state. So the probe logic would look like: 1) call ->detect to read physical connector status 2) return if physical status did not change 3) increment epoch counter 4) call ->update to update connector state and properties (EDID, etc) get new connector status 5) call ->get_modes if connected The initial ->detect would be minimal. The ->update, if implemented, could do more processing and error checking. It's result would be the connector's new status. On a side note, I've recently spend quite a few patches on getting the BMC output for ast and mgag200 usable. Something like the above logic would have helped, I think. Because with the current probe logic, I had to implement steps 1 to 4 in ->detect itself. The result has to maintain physical status and epoch counter by itself. [1] Best regards Thomas [1] https://gitlab.freedesktop.org/drm/kernel/-/commit/2a2391f857cdc5cf16f8df030944cef8d3d2bc30 > > BR, > Jani. > >
On Thu, 12 Sep 2024, Thomas Zimmermann <tzimmermann@suse.de> wrote: > Hi > > Am 12.09.24 um 13:25 schrieb Jani Nikula: >> On Thu, 12 Sep 2024, Thomas Zimmermann <tzimmermann@suse.de> wrote: >>> Hi >>> >>> Am 12.09.24 um 11:38 schrieb Jani Nikula: >>>> On Thu, 12 Sep 2024, Thomas Zimmermann <tzimmermann@suse.de> wrote: >>>>> Am 12.09.24 um 10:56 schrieb Jani Nikula: >>>>>> Moreover, in this case .detect() only detects digital displays as >>>>>> reported by EDID. If you postpone that to .get_modes(), the probe helper >>>>>> will still report connected, and invent non-EDID fallback modes. The >>>>>> behaviour changes. >>>>> The change in behavior is intentional, because the current test seems >>>>> arbitrary. Does the driver not work with analog outputs? >>>> Not on a DVI/HDMI port. Same with i915. >>>> >>>> That's possibly the only way to distinguish a DVI-A display connected to >>>> DVI-D source. >>> That's a detect failure, but IMHO our probe helpers should really handle >>> this case. >> How? Allow returning detect failures from .get_modes()? > > Something like that, I guess. > > For the specific problem it would be enough to read the first 20 bytes > of EDID data on DVI connectors and test the digital-input flag bit > against the exact connector requirements. drm_probe_ddc() could do this. Just a quick reply on this particular point: I'm very strongly against doing partial EDID reads. It's all geared towards EDID block sized handling. And you can't do checksum checking on the 20 bytes. It would be a maze of special casing, something the EDID code could have less, not more. BR, Jani. > Non-DVI connectors would continue to read a single bytes to detect the DDC. > > For more sophisticated problems, it would be good to introduce an > intermediate callback that updates the connector state. So the probe > logic would look like: > > 1) call ->detect to read physical connector status > 2) return if physical status did not change > 3) increment epoch counter > 4) call ->update to update connector state and properties (EDID, etc) > get new connector status > 5) call ->get_modes if connected > > The initial ->detect would be minimal. The ->update, if implemented, > could do more processing and error checking. It's result would be the > connector's new status. > > On a side note, I've recently spend quite a few patches on getting the > BMC output for ast and mgag200 usable. Something like the above logic > would have helped, I think. Because with the current probe logic, I had > to implement steps 1 to 4 in ->detect itself. The result has to maintain > physical status and epoch counter by itself. [1] > > Best regards > Thomas > > [1] > https://gitlab.freedesktop.org/drm/kernel/-/commit/2a2391f857cdc5cf16f8df030944cef8d3d2bc30 > >> >> BR, >> Jani. >> >>
On Thu, Sep 12, 2024 at 01:08:06PM +0200, Thomas Zimmermann wrote: > Hi > > Am 12.09.24 um 11:30 schrieb Jani Nikula: > > On Thu, 12 Sep 2024, Thomas Zimmermann <tzimmermann@suse.de> wrote: > >> Hi > >> > >> Am 12.09.24 um 10:48 schrieb Jani Nikula: > >>> On Thu, 12 Sep 2024, Thomas Zimmermann <tzimmermann@suse.de> wrote: > >>>> Hi > >>>> > >>>> Am 11.09.24 um 20:06 schrieb Tejas Vipin: > >>>>> Replace drm_detect_hdmi_monitor() with drm_display_info.is_hdmi since > >>>>> monitor HDMI information is available after EDID is parsed. Additionally > >>>>> rewrite the code the code to have fewer indentation levels. > >>>> The problem is that the entire logic is outdated. The content > >>>> of cdv_hdmi_detect() should go into cdv_hdmi_get_modes(), the detect_ctx > >>>> callback should be set to drm_connector_helper_detect_from_ddc() and > >>>> cdv_hdmi_detect() should be deleted. The result is that ->detect_ctx > >>>> will detect the presence of a display and ->get_modes will update EDID > >>>> and other properties. > >>> I guess I didn't get the memo on this one. > >>> > >>> What's the problem with reading the EDID at detect? The subsequent > >>> drm_edid_connector_add_modes() called from .get_modes() does not need to > >>> read the EDID again. > >> With drm_connector_helper_detect_from_ddc() there is already a helper > >> for detection. It makes sense to use it. And if we continue to update > >> the properties in detect (instead of get_modes), what is the correct > >> connector_status on errors? Right now and with the patch applied, detect > >> returns status_disconnected on errors. But this isn't correct if there > >> actually is a display. By separating detect and get_modes cleanly, we > >> can detect the display reliably, but also handle errors better than we > >> currently do in gma500. Get_modes is already expected to update the EDID > >> property, [1] for detect it's not so clear AFAICT. I think that from a > >> design perspective, it makes sense to have a read-only function that > >> only detects the physical state of the connector and a read-write > >> function that updates the connector's properties. Best regards Thomas > >> [1] > >> https://elixir.bootlin.com/linux/v6.10.9/source/include/drm/drm_modeset_helper_vtables.h#L865 > > So what if you can probe DDC but can't actually read an EDID of any > > kind? IMO that's a detect failure. > > Not being able to read the EDID is not a failure IMHO. It's better to > report a detected display and only provide minimal support, than to > outright reject it. The display is essential for most users being able > to use the computer at all, so it's often better to display something at > lower quality than display nothing at all. > > > > > Or how about things like CEC attach? Seems natural to do it at > > .detect(). Doing it at .get_modes() just seems wrong. However, it needs > > the EDID for physical address. > > > > I just don't think one size fits all here. > > The good thing about the EDID probe helpers is that it only reads a > minimal amount of data, like a single byte or the EDID header, so > something like that. Many drivers poll the DDC every 10 seconds via > ->detect. Which means that code running in ->detect possibly runs > concurrently to other DRM operations, such as page flips. Hence, > ->detect can interfere with the driver's hot path. The call to > ->get_modes usually only runs after the connector status changes to > connected, which rarely happens. It's also not time critical as no > modeset has happened yet. I don't see how you can really optimize polling with this because the only way to figure out if the EDID changed is to read it. Anyways, my gut feeling is that we need things in .detect() because that's also where DPCD is read, and we defintiely need that to do anything sensible. And dongles can also snoop the EDID reads and I think even potentially change their DPCD based on that, so having that go out of sync by skipping the EDID read might end up with weird behaviour.
On 9/12/24 12:49 PM, Thomas Zimmermann wrote: > Hi > > Am 11.09.24 um 20:06 schrieb Tejas Vipin: >> Replace drm_detect_hdmi_monitor() with drm_display_info.is_hdmi since >> monitor HDMI information is available after EDID is parsed. Additionally >> rewrite the code the code to have fewer indentation levels. > > The problem is that the entire logic is outdated. The content of cdv_hdmi_detect() should go into cdv_hdmi_get_modes(), the detect_ctx callback should be set to drm_connector_helper_detect_from_ddc() and cdv_hdmi_detect() should be deleted. The result is that ->detect_ctx will detect the presence of a display and ->get_modes will update EDID and other properties. > > Do you have a device for testing such a change? > > Best regards > Thomas I do not have a device to test this. Reading the rest of the series and given my circumstances, I do not think I will be continuing with this patch.
diff --git a/drivers/gpu/drm/gma500/cdv_intel_hdmi.c b/drivers/gpu/drm/gma500/cdv_intel_hdmi.c index 2d95e0471291..701f8bbd5f2b 100644 --- a/drivers/gpu/drm/gma500/cdv_intel_hdmi.c +++ b/drivers/gpu/drm/gma500/cdv_intel_hdmi.c @@ -128,23 +128,25 @@ static enum drm_connector_status cdv_hdmi_detect( { struct gma_encoder *gma_encoder = gma_attached_encoder(connector); struct mid_intel_hdmi_priv *hdmi_priv = gma_encoder->dev_priv; - struct edid *edid = NULL; + const struct drm_edid *drm_edid; + int ret; enum drm_connector_status status = connector_status_disconnected; - edid = drm_get_edid(connector, connector->ddc); + drm_edid = drm_edid_read_ddc(connector, connector->ddc); + ret = drm_edid_connector_update(connector, drm_edid); hdmi_priv->has_hdmi_sink = false; hdmi_priv->has_hdmi_audio = false; - if (edid) { - if (edid->input & DRM_EDID_INPUT_DIGITAL) { - status = connector_status_connected; - hdmi_priv->has_hdmi_sink = - drm_detect_hdmi_monitor(edid); - hdmi_priv->has_hdmi_audio = - drm_detect_monitor_audio(edid); - } - kfree(edid); + if (ret) + return status; + + if (drm_edid_is_digital(drm_edid)) { + status = connector_status_connected; + hdmi_priv->has_hdmi_sink = connector->display_info.is_hdmi; + hdmi_priv->has_hdmi_audio = connector->display_info.has_audio; } + drm_edid_free(drm_edid); + return status; }
Replace drm_detect_hdmi_monitor() with drm_display_info.is_hdmi since monitor HDMI information is available after EDID is parsed. Additionally rewrite the code the code to have fewer indentation levels. Signed-off-by: Tejas Vipin <tejasvipin76@gmail.com> --- Changes in v2: - Use drm_edid instead of edid Link to v1: https://lore.kernel.org/all/20240910051856.700210-1-tejasvipin76@gmail.com/ --- drivers/gpu/drm/gma500/cdv_intel_hdmi.c | 24 +++++++++++++----------- 1 file changed, 13 insertions(+), 11 deletions(-)