| Message ID | 20240319081430.10165-13-tzimmermann@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/ast: Detect connector status for VGA and SIL164 | expand |
Hi, On Tue, Mar 19, 2024 at 09:00:32AM +0100, Thomas Zimmermann wrote: > Implement polling for VGA and SIL164 connectors. Set the flag > DRM_CONNECTOR_POLL_DISCONNECT for each to detect the removal of the > monitor cable. Implement struct drm_connector_helper_funcs.detect_ctx > for each type of connector by testing for EDID data. The code for > both types of connectors is identical for now. Maybe this can later > become a common helper function for various drivers. > > Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> > --- > drivers/gpu/drm/ast/ast_mode.c | 36 ++++++++++++++++++++++++++++++++-- > 1 file changed, 34 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c > index 71cc681d6188f..f740b8706a38b 100644 > --- a/drivers/gpu/drm/ast/ast_mode.c > +++ b/drivers/gpu/drm/ast/ast_mode.c > @@ -1344,8 +1344,24 @@ static int ast_crtc_init(struct drm_device *dev) > * VGA Connector > */ > > +static int ast_vga_connector_helper_detect_ctx(struct drm_connector *connector, > + struct drm_modeset_acquire_ctx *ctx, > + bool force) > +{ > + enum drm_connector_status status = connector_status_disconnected; > + const struct drm_edid *edid; > + > + edid = drm_edid_read(connector); > + if (edid) > + status = connector_status_connected; > + drm_edid_free(edid); > + > + return status; > +} > + Yeah, I think it would be worth turning it into a helper. We have a number of drivers using some variation of that already (display-connector, loongson). It's probably better to use drm_probe_ddc here too instead of parsing and updating the EDID property each time we call detect. Maxime
Hi Am 19.03.24 um 10:37 schrieb Maxime Ripard: > Hi, > > On Tue, Mar 19, 2024 at 09:00:32AM +0100, Thomas Zimmermann wrote: >> Implement polling for VGA and SIL164 connectors. Set the flag >> DRM_CONNECTOR_POLL_DISCONNECT for each to detect the removal of the >> monitor cable. Implement struct drm_connector_helper_funcs.detect_ctx >> for each type of connector by testing for EDID data. The code for >> both types of connectors is identical for now. Maybe this can later >> become a common helper function for various drivers. >> >> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> >> --- >> drivers/gpu/drm/ast/ast_mode.c | 36 ++++++++++++++++++++++++++++++++-- >> 1 file changed, 34 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c >> index 71cc681d6188f..f740b8706a38b 100644 >> --- a/drivers/gpu/drm/ast/ast_mode.c >> +++ b/drivers/gpu/drm/ast/ast_mode.c >> @@ -1344,8 +1344,24 @@ static int ast_crtc_init(struct drm_device *dev) >> * VGA Connector >> */ >> >> +static int ast_vga_connector_helper_detect_ctx(struct drm_connector *connector, >> + struct drm_modeset_acquire_ctx *ctx, >> + bool force) >> +{ >> + enum drm_connector_status status = connector_status_disconnected; >> + const struct drm_edid *edid; >> + >> + edid = drm_edid_read(connector); >> + if (edid) >> + status = connector_status_connected; >> + drm_edid_free(edid); >> + >> + return status; >> +} >> + > Yeah, I think it would be worth turning it into a helper. We have a > number of drivers using some variation of that already > (display-connector, loongson). > > It's probably better to use drm_probe_ddc here too instead of parsing > and updating the EDID property each time we call detect. I see. I'll update the patch accordingly. drm_probe_ddc() doesn't update the property, which is probably better anyway. Best regard Thomas > > Maxime
On Tue, 19 Mar 2024, Thomas Zimmermann <tzimmermann@suse.de> wrote: > Hi > > Am 19.03.24 um 10:37 schrieb Maxime Ripard: >> Hi, >> >> On Tue, Mar 19, 2024 at 09:00:32AM +0100, Thomas Zimmermann wrote: >>> Implement polling for VGA and SIL164 connectors. Set the flag >>> DRM_CONNECTOR_POLL_DISCONNECT for each to detect the removal of the >>> monitor cable. Implement struct drm_connector_helper_funcs.detect_ctx >>> for each type of connector by testing for EDID data. The code for >>> both types of connectors is identical for now. Maybe this can later >>> become a common helper function for various drivers. >>> >>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> >>> --- >>> drivers/gpu/drm/ast/ast_mode.c | 36 ++++++++++++++++++++++++++++++++-- >>> 1 file changed, 34 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c >>> index 71cc681d6188f..f740b8706a38b 100644 >>> --- a/drivers/gpu/drm/ast/ast_mode.c >>> +++ b/drivers/gpu/drm/ast/ast_mode.c >>> @@ -1344,8 +1344,24 @@ static int ast_crtc_init(struct drm_device *dev) >>> * VGA Connector >>> */ >>> >>> +static int ast_vga_connector_helper_detect_ctx(struct drm_connector *connector, >>> + struct drm_modeset_acquire_ctx *ctx, >>> + bool force) >>> +{ >>> + enum drm_connector_status status = connector_status_disconnected; >>> + const struct drm_edid *edid; >>> + >>> + edid = drm_edid_read(connector); >>> + if (edid) >>> + status = connector_status_connected; >>> + drm_edid_free(edid); >>> + >>> + return status; >>> +} >>> + >> Yeah, I think it would be worth turning it into a helper. We have a >> number of drivers using some variation of that already >> (display-connector, loongson). >> >> It's probably better to use drm_probe_ddc here too instead of parsing >> and updating the EDID property each time we call detect. > > I see. I'll update the patch accordingly. drm_probe_ddc() doesn't update > the property, which is probably better anyway. The struct drm_edid based drm_edid_read() and friends do *not* parse the EDID (apart from what's necessary to read the EDID) nor update the EDID property. You need to call drm_edid_connector_update() separately for that. This is by design. The struct edid based drm_get_edid() does parse and update the property, and I think adding that was a mistake that a lot of drivers later depended on, and couldn't be removed. As a design consideration, IMO it's also a fine approach to read and cache the EDID and update the property at ->detect, and then use drm_edid_connector_add_modes() in ->get_modes to avoid re-reading the EDID at that time. It's not uncommon to need stuff from the EDID between those hooks. BR, Jani.
On Tue, 19 Mar 2024, Jani Nikula <jani.nikula@linux.intel.com> wrote: > On Tue, 19 Mar 2024, Thomas Zimmermann <tzimmermann@suse.de> wrote: >> Hi >> >> Am 19.03.24 um 10:37 schrieb Maxime Ripard: >>> Hi, >>> >>> On Tue, Mar 19, 2024 at 09:00:32AM +0100, Thomas Zimmermann wrote: >>>> Implement polling for VGA and SIL164 connectors. Set the flag >>>> DRM_CONNECTOR_POLL_DISCONNECT for each to detect the removal of the >>>> monitor cable. Implement struct drm_connector_helper_funcs.detect_ctx >>>> for each type of connector by testing for EDID data. The code for >>>> both types of connectors is identical for now. Maybe this can later >>>> become a common helper function for various drivers. >>>> >>>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> >>>> --- >>>> drivers/gpu/drm/ast/ast_mode.c | 36 ++++++++++++++++++++++++++++++++-- >>>> 1 file changed, 34 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c >>>> index 71cc681d6188f..f740b8706a38b 100644 >>>> --- a/drivers/gpu/drm/ast/ast_mode.c >>>> +++ b/drivers/gpu/drm/ast/ast_mode.c >>>> @@ -1344,8 +1344,24 @@ static int ast_crtc_init(struct drm_device *dev) >>>> * VGA Connector >>>> */ >>>> >>>> +static int ast_vga_connector_helper_detect_ctx(struct drm_connector *connector, >>>> + struct drm_modeset_acquire_ctx *ctx, >>>> + bool force) >>>> +{ >>>> + enum drm_connector_status status = connector_status_disconnected; >>>> + const struct drm_edid *edid; >>>> + >>>> + edid = drm_edid_read(connector); >>>> + if (edid) >>>> + status = connector_status_connected; >>>> + drm_edid_free(edid); >>>> + >>>> + return status; >>>> +} >>>> + >>> Yeah, I think it would be worth turning it into a helper. We have a >>> number of drivers using some variation of that already >>> (display-connector, loongson). >>> >>> It's probably better to use drm_probe_ddc here too instead of parsing >>> and updating the EDID property each time we call detect. >> >> I see. I'll update the patch accordingly. drm_probe_ddc() doesn't update >> the property, which is probably better anyway. > > The struct drm_edid based drm_edid_read() and friends do *not* parse the > EDID (apart from what's necessary to read the EDID) nor update the EDID > property. You need to call drm_edid_connector_update() separately for > that. This is by design. > > The struct edid based drm_get_edid() does parse and update the property, > and I think adding that was a mistake that a lot of drivers later > depended on, and couldn't be removed. > > As a design consideration, IMO it's also a fine approach to read and > cache the EDID and update the property at ->detect, and then use > drm_edid_connector_add_modes() in ->get_modes to avoid re-reading the > EDID at that time. It's not uncommon to need stuff from the EDID between > those hooks. But if drm_probe_ddc() at ->detect and drm_connector_helper_get_modes() for ->get_modes are sufficient, nothing wrong with that either. BR, Jani. > > > BR, > Jani.
Hi Am 19.03.24 um 11:08 schrieb Jani Nikula: > On Tue, 19 Mar 2024, Thomas Zimmermann <tzimmermann@suse.de> wrote: >> Hi >> >> Am 19.03.24 um 10:37 schrieb Maxime Ripard: >>> Hi, >>> >>> On Tue, Mar 19, 2024 at 09:00:32AM +0100, Thomas Zimmermann wrote: >>>> Implement polling for VGA and SIL164 connectors. Set the flag >>>> DRM_CONNECTOR_POLL_DISCONNECT for each to detect the removal of the >>>> monitor cable. Implement struct drm_connector_helper_funcs.detect_ctx >>>> for each type of connector by testing for EDID data. The code for >>>> both types of connectors is identical for now. Maybe this can later >>>> become a common helper function for various drivers. >>>> >>>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> >>>> --- >>>> drivers/gpu/drm/ast/ast_mode.c | 36 ++++++++++++++++++++++++++++++++-- >>>> 1 file changed, 34 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c >>>> index 71cc681d6188f..f740b8706a38b 100644 >>>> --- a/drivers/gpu/drm/ast/ast_mode.c >>>> +++ b/drivers/gpu/drm/ast/ast_mode.c >>>> @@ -1344,8 +1344,24 @@ static int ast_crtc_init(struct drm_device *dev) >>>> * VGA Connector >>>> */ >>>> >>>> +static int ast_vga_connector_helper_detect_ctx(struct drm_connector *connector, >>>> + struct drm_modeset_acquire_ctx *ctx, >>>> + bool force) >>>> +{ >>>> + enum drm_connector_status status = connector_status_disconnected; >>>> + const struct drm_edid *edid; >>>> + >>>> + edid = drm_edid_read(connector); >>>> + if (edid) >>>> + status = connector_status_connected; >>>> + drm_edid_free(edid); >>>> + >>>> + return status; >>>> +} >>>> + >>> Yeah, I think it would be worth turning it into a helper. We have a >>> number of drivers using some variation of that already >>> (display-connector, loongson). >>> >>> It's probably better to use drm_probe_ddc here too instead of parsing >>> and updating the EDID property each time we call detect. >> I see. I'll update the patch accordingly. drm_probe_ddc() doesn't update >> the property, which is probably better anyway. > The struct drm_edid based drm_edid_read() and friends do *not* parse the > EDID (apart from what's necessary to read the EDID) nor update the EDID > property. You need to call drm_edid_connector_update() separately for > that. This is by design. Right. I'm confusing things. Sorry. Best regards Thomas > > The struct edid based drm_get_edid() does parse and update the property, > and I think adding that was a mistake that a lot of drivers later > depended on, and couldn't be removed. > > As a design consideration, IMO it's also a fine approach to read and > cache the EDID and update the property at ->detect, and then use > drm_edid_connector_add_modes() in ->get_modes to avoid re-reading the > EDID at that time. It's not uncommon to need stuff from the EDID between > those hooks. > > > BR, > Jani. > >
diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c index 71cc681d6188f..f740b8706a38b 100644 --- a/drivers/gpu/drm/ast/ast_mode.c +++ b/drivers/gpu/drm/ast/ast_mode.c @@ -1344,8 +1344,24 @@ static int ast_crtc_init(struct drm_device *dev) * VGA Connector */ +static int ast_vga_connector_helper_detect_ctx(struct drm_connector *connector, + struct drm_modeset_acquire_ctx *ctx, + bool force) +{ + enum drm_connector_status status = connector_status_disconnected; + const struct drm_edid *edid; + + edid = drm_edid_read(connector); + if (edid) + status = connector_status_connected; + drm_edid_free(edid); + + return status; +} + static const struct drm_connector_helper_funcs ast_vga_connector_helper_funcs = { .get_modes = drm_connector_helper_get_modes, + .detect_ctx = ast_vga_connector_helper_detect_ctx, }; static const struct drm_connector_funcs ast_vga_connector_funcs = { @@ -1379,7 +1395,7 @@ static int ast_vga_connector_init(struct drm_device *dev, struct drm_connector * connector->interlace_allowed = 0; connector->doublescan_allowed = 0; - connector->polled = DRM_CONNECTOR_POLL_CONNECT; + connector->polled = DRM_CONNECTOR_POLL_CONNECT | DRM_CONNECTOR_POLL_DISCONNECT; return 0; } @@ -1412,8 +1428,24 @@ static int ast_vga_output_init(struct ast_device *ast) * SIL164 Connector */ +static int ast_sil164_connector_helper_detect_ctx(struct drm_connector *connector, + struct drm_modeset_acquire_ctx *ctx, + bool force) +{ + enum drm_connector_status status = connector_status_disconnected; + const struct drm_edid *edid; + + edid = drm_edid_read(connector); + if (edid) + status = connector_status_connected; + drm_edid_free(edid); + + return status; +} + static const struct drm_connector_helper_funcs ast_sil164_connector_helper_funcs = { .get_modes = drm_connector_helper_get_modes, + .detect_ctx = ast_sil164_connector_helper_detect_ctx, }; static const struct drm_connector_funcs ast_sil164_connector_funcs = { @@ -1447,7 +1479,7 @@ static int ast_sil164_connector_init(struct drm_device *dev, struct drm_connecto connector->interlace_allowed = 0; connector->doublescan_allowed = 0; - connector->polled = DRM_CONNECTOR_POLL_CONNECT; + connector->polled = DRM_CONNECTOR_POLL_CONNECT | DRM_CONNECTOR_POLL_DISCONNECT; return 0; }
Implement polling for VGA and SIL164 connectors. Set the flag DRM_CONNECTOR_POLL_DISCONNECT for each to detect the removal of the monitor cable. Implement struct drm_connector_helper_funcs.detect_ctx for each type of connector by testing for EDID data. The code for both types of connectors is identical for now. Maybe this can later become a common helper function for various drivers. Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> --- drivers/gpu/drm/ast/ast_mode.c | 36 ++++++++++++++++++++++++++++++++-- 1 file changed, 34 insertions(+), 2 deletions(-)