Re: [Intel-gfx] [PATCH v1 06/13] drm/probe-helper: make .get_modes() optional, add default action

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

On Thu, 02 Jun 2022, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> On Tue, May 24, 2022 at 01:39:28PM +0300, Jani Nikula wrote:
>> Add default action when .get_modes() not set. This also defines what a
>> .get_modes() hook should do.
>> 
>> Cc: David Airlie <airlied@linux.ie>
>> Cc: Daniel Vetter <daniel@ffwll.ch>
>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> ---
>>  drivers/gpu/drm/drm_probe_helper.c       | 14 +++++++++++++-
>>  include/drm/drm_modeset_helper_vtables.h |  4 ++++
>>  2 files changed, 17 insertions(+), 1 deletion(-)
>> 
>> diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
>> index 836bcd5b4cb6..9df17f0ae225 100644
>> --- a/drivers/gpu/drm/drm_probe_helper.c
>> +++ b/drivers/gpu/drm/drm_probe_helper.c
>> @@ -360,7 +360,19 @@ static int drm_helper_probe_get_modes(struct drm_connector *connector)
>>  		connector->helper_private;
>>  	int count;
>>  
>> -	count = connector_funcs->get_modes(connector);
>> +	if (connector_funcs->get_modes) {
>> +		count = connector_funcs->get_modes(connector);
>> +	} else {
>> +		const struct drm_edid *drm_edid;
>> +
>> +		/* Note: This requires connector->ddc is set */
>> +		drm_edid = drm_edid_read(connector);
>> +
>> +		/* Update modes etc. from the EDID */
>> +		count = drm_edid_connector_update(connector, drm_edid);
>> +
>> +		drm_edid_free(drm_edid);
>> +	}
>
> Not really a huge fan of this automagic fallback. I think I'd prefer
> to keep it mandatory and just provide this as a helper for drivers to
> plug into the right spot. Otherwise I'm sure I'll forget how this works
> and then I'll be confused when I see a connector withput any apparent
> .get_modes() implementation.

Fair enough.

I'm not sure how useful that is going to be, though, at least not for
i915. If we add a .get_edid hook, where would you bolt that? It doesn't
feel right to set a .get_modes hook to a default function that goes on
to call a .get_edid hook. Or what do you think?

BR,
Jani.

>
>>  
>>  	/*
>>  	 * Fallback for when DDC probe failed in drm_get_edid() and thus skipped
>> diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h
>> index fafa70ac1337..b4402bc64e57 100644
>> --- a/include/drm/drm_modeset_helper_vtables.h
>> +++ b/include/drm/drm_modeset_helper_vtables.h
>> @@ -894,6 +894,10 @@ struct drm_connector_helper_funcs {
>>  	 * libraries always call this with the &drm_mode_config.connection_mutex
>>  	 * held. Because of this it's safe to inspect &drm_connector->state.
>>  	 *
>> +	 * This callback is optional. By default, it reads the EDID using
>> +	 * drm_edid_read() and updates the connector display info, modes, and
>> +	 * properties using drm_edid_connector_update().
>> +	 *
>>  	 * RETURNS:
>>  	 *
>>  	 * The number of modes added by calling drm_mode_probed_add().
>> -- 
>> 2.30.2

-- 
Jani Nikula, Intel Open Source Graphics Center