Re: [PATCH v5 9/9] drm/vc4: hdmi: use drm_atomic_helper_connector_hdmi_update_edid()

[Jani Nikula <jani.nikula@linux.intel.com>]

On Tue, 03 Dec 2024, Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote:
> On Mon, Dec 02, 2024 at 02:27:49PM +0100, Maxime Ripard wrote:
>> Hi,
>> 
>> On Sun, Dec 01, 2024 at 02:44:13AM +0200, Dmitry Baryshkov wrote:
>> > Use the helper function to update the connector's information. This
>> > makes sure that HDMI-related events are handled in a generic way.
>> > Currently it is limited to the HDMI state reporting to the sound system.
>> > 
>> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>> > ---
>> >  drivers/gpu/drm/vc4/vc4_hdmi.c | 9 +++++++--
>> >  1 file changed, 7 insertions(+), 2 deletions(-)
>> > 
>> > diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
>> > index d0a9aff7ad43016647493263c00d593296a1e3ad..d83f587ab69f4b8f7d5c37a00777f11da8301bc1 100644
>> > --- a/drivers/gpu/drm/vc4/vc4_hdmi.c
>> > +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
>> > @@ -401,13 +401,16 @@ static void vc4_hdmi_handle_hotplug(struct vc4_hdmi *vc4_hdmi,
>> >  	 */
>> >  
>> >  	if (status == connector_status_disconnected) {
>> > +		drm_atomic_helper_connector_hdmi_update_edid(connector, NULL);
>> >  		cec_phys_addr_invalidate(vc4_hdmi->cec_adap);
>> >  		return;
>> >  	}
>> >  
>> >  	drm_edid = drm_edid_read_ddc(connector, vc4_hdmi->ddc);
>> >  
>> > -	drm_edid_connector_update(connector, drm_edid);
>> > +	// TODO: use drm_atomic_helper_connector_hdmi_update() once it gains
>> > +	// CEC support
>> > +	drm_atomic_helper_connector_hdmi_update_edid(connector, drm_edid);
>> 
>> So, it's not just about EDID, and I think we shouldn't really focus on
>> that either.
>> 
>> As that patch points out, even if we only consider EDID's, we have
>> different path depending on the connector status. It shouldn't be up to
>> the drivers to get this right.
>> 
>> What I had in mind was something like a
>> drm_atomic_helper_connector_hdmi_hotplug function that would obviously
>> deal with EDID only here, but would expand to CEC, scrambling, etc.
>> later on.
>
> I thought about it, after our discussion, but in the end I had to
> implement the EDID-specific function, using edid == NULL as
> "disconnected" event. The issue is pretty simple: there is no standard
> way to get EDID from the connector. The devices can call
> drm_edid_read(), drm_edid_read_ddc(connector->ddc) or (especially
> embedded bridges) use drm_edid_read_custom().

Lack of EDID may be a good approximation of disconnected for displays
that should have one, but just a reminder that having an EDID should not
be the only approximation for connected.

This is relevant for the debugfs/firmware EDID case. You'll want to be
able to have that, without it implying connected.

BR,
Jani.


>
> Of course we can go with the functional way and add the
> .read_edid(drm_connector) callback to the HDMI funcs. Then the
> drm_atomic_helper_connector_hdmi_hotplug() function can read EDID on its
> own.
>
> Also the function that you proposed perfectly fits the HPD notification
> callbacks, but which function should be called from the .get_modes()?
> The _hdmi_hotplug() doesn't fit there. Do we still end up with both
> drm_atomic_helper_connector_hdmi_hotplug() and
> drm_atomic_helper_connector_hdmi_update_edid()?
>
>> 
>> And would cover both the connected/disconnected cases.
>> 
>> Maxime

-- 
Jani Nikula, Intel