Re: [PATCH 1/2] drm: Introduce crtc->mode_valid() callback

[Jose Abreu]

Hi Daniel,


On 02-05-2017 09:48, Daniel Vetter wrote:
> On Wed, Apr 26, 2017 at 11:48:34AM +0100, Jose Abreu wrote:
>> Some crtc's may have restrictions in the mode they can display. In
>> this patch a new callback (crtc->mode_valid()) is introduced that
>> is called at the same stage of connector->mode_valid() callback.
>>
>> This shall be implemented if the crtc has some sort of restriction
>> so that we don't probe modes that will fail in the commit() stage.
>> For example: A given crtc may be responsible to set a clock value.
>> If the clock can not produce all the values for the available
>> modes then this callback can be used to restrict the number of
>> probbed modes to only the ones that can be displayed.
>>
>> If the crtc does not implement the callback then the behaviour will
>> remain the same. Also, for a given set of crtcs that can be bound to
>> the connector, if at least one can display the mode then the mode
>> will be probbed.
>>
>> Signed-off-by: Jose Abreu <joabreu@xxxxxxxxxxxx>
>> Cc: Carlos Palminha <palminha@xxxxxxxxxxxx>
>> Cc: Alexey Brodkin <abrodkin@xxxxxxxxxxxx>
>> Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
>> Cc: Daniel Vetter <daniel.vetter@xxxxxxxx>
>> Cc: Dave Airlie <airlied@xxxxxxxx>
> Not sure this is useful, since you still have to duplicate the exact same
> check into your ->mode_fixup hook. That seems to make things even more
> confusing.

Yeah, in arcpgu I had to duplicate the code in ->atomic_check.

>
> Also this doesn't update the various kerneldoc comments. For the existing
> hooks. Since this topic causes so much confusion, I don't think a
> half-solution will help, but has some good potential to make things worse.

I only documented the callback in drm_modeset_helper_vtables.h.

Despite all of this, I think it doesn't makes sense delivering
modes to userspace which can never be used.

This is really annoying in arcpgu. Imagine: I try to use mpv to
play a video, the full set of modes from EDID were probed so if I
just start mpv it will pick the native mode of the TV instead of
the one that is supported, so mpv will fail to play. I know the
value of clock which will work (so I know what mode shall be
used), but a normal user which is not aware of the HW will have
to cycle through the list of modes and try them all until it hits
one that works. Its really boring.

For the modes that user specifies manually there is nothing we
can do, but we should not trick users into thinking that a given
mode is supported when it will always fail at commit.

Best regards,
Jose Miguel Abreu

> -Daniel
>
>> ---
>>  drivers/gpu/drm/drm_probe_helper.c       | 44 ++++++++++++++++++++++++++++++++
>>  include/drm/drm_modeset_helper_vtables.h | 26 +++++++++++++++++++
>>  2 files changed, 70 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
>> index 1b0c14a..61eac30 100644
>> --- a/drivers/gpu/drm/drm_probe_helper.c
>> +++ b/drivers/gpu/drm/drm_probe_helper.c
>> @@ -80,6 +80,46 @@
>>  	return MODE_OK;
>>  }
>>  
>> +static enum drm_mode_status drm_mode_validate_connector_crtc(
>> +		struct drm_connector *connector,
>> +		struct drm_display_mode *mode)
>> +{
>> +	const struct drm_crtc_helper_funcs *crtc_funcs = NULL;
>> +	enum drm_mode_status mode_status = MODE_ERROR;
>> +	struct drm_device *dev = connector->dev;
>> +	struct drm_encoder *encoder;
>> +	struct drm_crtc *crtc;
>> +	bool callback_found = false;
>> +	int i;
>> +
>> +	for (i = 0; i < DRM_CONNECTOR_MAX_ENCODER; i++) {
>> +		encoder = drm_encoder_find(dev, connector->encoder_ids[i]);
>> +
>> +		if (!encoder)
>> +			continue;
>> +
>> +		drm_for_each_crtc(crtc, dev) {
>> +			crtc_funcs = crtc->helper_private;
>> +
>> +			if (!drm_encoder_crtc_ok(encoder, crtc))
>> +				continue;
>> +			if (!crtc_funcs || !crtc_funcs->mode_valid)
>> +				continue;
>> +
>> +			/* MODE_OK=0 and default mode_status=MODE_ERROR=-1
>> +			 * so if at least one crtc accepts the mode we get
>> +			 * MODE_OK */
>> +			mode_status &= crtc_funcs->mode_valid(crtc, mode);
>> +			callback_found |= true;
>> +		}
>> +	}
>> +
>> +	/* We can reach here without calling mode_valid if there is no
>> +	 * registered crtc with this callback, lets return MODE_OK in this
>> +	 * case */
>> +	return callback_found ? mode_status : MODE_OK;
>> +}
>> +
>>  static int drm_helper_probe_add_cmdline_mode(struct drm_connector *connector)
>>  {
>>  	struct drm_cmdline_mode *cmdline_mode;
>> @@ -431,6 +471,10 @@ int drm_helper_probe_single_connector_modes(struct drm_connector *connector,
>>  		if (mode->status == MODE_OK && connector_funcs->mode_valid)
>>  			mode->status = connector_funcs->mode_valid(connector,
>>  								   mode);
>> +
>> +		if (mode->status == MODE_OK)
>> +			mode->status = drm_mode_validate_connector_crtc(
>> +					connector, mode);
>>  	}
>>  
>>  prune:
>> diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h
>> index c01c328..59fffba 100644
>> --- a/include/drm/drm_modeset_helper_vtables.h
>> +++ b/include/drm/drm_modeset_helper_vtables.h
>> @@ -106,6 +106,32 @@ struct drm_crtc_helper_funcs {
>>  	void (*commit)(struct drm_crtc *crtc);
>>  
>>  	/**
>> +	 * @mode_valid:
>> +	 *
>> +	 * This callback should be implemented if the crtc has some sort of
>> +	 * restriction in the modes it can display. For example, a given crtc
>> +	 * may be responsible to set a clock value. If the clock can not
>> +	 * produce all the values for the available modes then this callback
>> +	 * can be used to restrict the number of probbed modes to only the ones
>> +	 * that can be displayed.
>> +	 *
>> +	 * This is directly called at the same stage of connector->mode_valid
> &drm_connector_helper_funcs.mode_valid to make a proper link. Please also
> check the output of make htmldocs and make sure it looks pretty.
>
>> +	 * callback.
>> +	 *
>> +	 * NOTE:
>> +	 *
>> +	 * For a given set of crtc's in a drm_device, if at least one does not
>> +	 * have the mode_valid callback, or, at least one returns MODE_OK then
>> +	 * the mode will be probbed.
>> +	 *
>> +	 * RETURNS:
>> +	 *
>> +	 * drm_mode_status Enum
>> +	 */
>> +	enum drm_mode_status (*mode_valid)(struct drm_crtc *crtc,
>> +			struct drm_display_mode *mode);
>> +
>> +	/**
>>  	 * @mode_fixup:
>>  	 *
>>  	 * This callback is used to validate a mode. The parameter mode is the
>> -- 
>> 1.9.1
>>
>>