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

Commit Message

Jose Abreu April 26, 2017, 10:48 a.m. UTC

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 <[e-mail]>
Cc: Carlos Palminha <[e-mail]>
Cc: Alexey Brodkin <[e-mail]>
Cc: Ville Syrjälä <[e-mail]>
Cc: Daniel Vetter <[e-mail]>
Cc: Dave Airlie <[e-mail]>
---
 drivers/gpu/drm/drm_probe_helper.c       | 44 ++++++++++++++++++++++++++++++++
 include/drm/drm_modeset_helper_vtables.h | 26 +++++++++++++++++++
 2 files changed, 70 insertions(+)

Comments

Andrzej Hajda April 27, 2017, 10:05 a.m. UTC | #1

Hi Jose,

On 26.04.2017 12:48, 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.

I see few possible issues/improvements here:
1. crtc can have different constraints depending on the encoder it is
connected to, theoretically reverse dependency is also possible, but I
am not aware of such hw.
2. there also could be similar dependency constrains between connector
and encoder, I guess.
3. encoders and bridges should have also possibility to validate modes,
they also have constrains, btw encoder_slave have such callback.

Regarding 1st and 2nd point, maybe it would be good to validate modes
according to topology described in drm_mode_get_connector::encoder_id
and drm_mode_get_encoder::crtc_id:
a) if connector is not connected to any encoder report to userspace
modes filtered only by connector::mode_valid,
b) if connector is connected to encoder, report modes filtered by
connector, encoder and attached bridges,
c) if full pipeline is constructed, report modes filtered by all members
of the pipeline.

Of course with this approach drm_mode_get_connector userspace should be
aware of the fact that it should re-call drm_mode_get_connector after
topology change to update supported modes.

Regards
Andrzej

Jose Abreu April 27, 2017, 12:34 p.m. UTC | #2

Hi Andrzej,


Thanks for your answer!


On 27-04-2017 11:05, Andrzej Hajda wrote:
> Hi Jose,
>
> On 26.04.2017 12:48, 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.
> I see few possible issues/improvements here:
> 1. crtc can have different constraints depending on the encoder it is
> connected to, theoretically reverse dependency is also possible, but I
> am not aware of such hw.
> 2. there also could be similar dependency constrains between connector
> and encoder, I guess.
> 3. encoders and bridges should have also possibility to validate modes,
> they also have constrains, btw encoder_slave have such callback.
>
> Regarding 1st and 2nd point, maybe it would be good to validate modes
> according to topology described in drm_mode_get_connector::encoder_id
> and drm_mode_get_encoder::crtc_id:
> a) if connector is not connected to any encoder report to userspace
> modes filtered only by connector::mode_valid,
> b) if connector is connected to encoder, report modes filtered by
> connector, encoder and attached bridges,
> c) if full pipeline is constructed, report modes filtered by all members
> of the pipeline.

I see your point but when the full pipeline is created we already
handed the modes to userspace, right?

This patch is simple and does not take into account that: it just
iterates over all crtcs that can be bound to the set of encoders
which instead can be bound to the given connector. So, we are
filtering modes from the connector but not taking into account
the pipeline. It will work for a single pipeline, but with
multiple constraints, just like you mentioned, it will probably
make no difference.

Despite this, I think its a good initial step.

>
> Of course with this approach drm_mode_get_connector userspace should be
> aware of the fact that it should re-call drm_mode_get_connector after
> topology change to update supported modes.

I believe we should avoid this, but indeed it seems necessary.

What about a new get_modes(encoder_id, connector_id, crtc_id)
ioctl ? User could call this and know what modes a given pipeline
will support.

Best regards,
Jose Miguel Abreu

>
> Regards
> Andrzej
>

Ville Syrjälä April 28, 2017, 11:41 a.m. UTC | #3

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 <[e-mail]>
> Cc: Carlos Palminha <[e-mail]>
> Cc: Alexey Brodkin <[e-mail]>
> Cc: Ville Syrjälä <[e-mail]>
> Cc: Daniel Vetter <[e-mail]>
> Cc: Dave Airlie <[e-mail]>
> ---
>  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)

Probably more typical way to split the lines would be:
static enum drm_mode_status
drm_mode_validate_connector_crtc(struct drm_connector *connector,
				 struct drm_display_mode *mode)

Also 'mode' should be const. Looks like the connector->mode_valid()
hook is missing the const as well, so that too should be fixed.

Not sure about the name of the function. It doesn't really reflect what
it does in the best way possible. drm_mode_validate_connector_possible_crtcs()
perhaps? But that is getting quite long so not sure if it' a good idea
either. I guess simply making the name refer to plural crtcs might be
a reasonable compromise, ie. drm_mode_validate_connector_crtcs()?

> +{
> +	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;

A lot of these can be moved into tighter scope.

> +	bool callback_found = false;

I don't think you need this variable at all. See below.

> +	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;

The need for a comment here tells me that it's probably better
to got for a more straightforward code. Something like:

mode_status = crtc_funcs->mode_valid(crtc, mode);
if (mode_statys == MODE_OK)
	return mode_status;

And at the end of the function just return MODE_ERROR, or
some other error value if we have something suitable. Hmm.
Perhaps we should just return the error from the first or last
crtc? Either should be pretty easy, just "ret = mode_status" within
the loop if it didn't return MODE_OK and then 'return ret' at the
end of the function.

> +		}
> +	}
> +
> +	/* 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);

Indentation.

And actually, maybe you should also move the connector_funcs->mode_valid()
call into the new function, and then you could just call the new
function drm_mode_validate_connector() or something along those lines.

>  	}
>  
>  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
> +	 * 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);

Const. Indentation looks off again.

> +
> +	/**
>  	 * @mode_fixup:
>  	 *
>  	 * This callback is used to validate a mode. The parameter mode is the
> -- 
> 1.9.1
>

Jose Abreu April 28, 2017, 12:30 p.m. UTC | #4

Hi Ville,


Thanks for the review! My comments inline.


On 28-04-2017 12:41, Ville Syrjälä 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 <[e-mail]>
>> Cc: Carlos Palminha <[e-mail]>
>> Cc: Alexey Brodkin <[e-mail]>
>> Cc: Ville Syrjälä <[e-mail]>
>> Cc: Daniel Vetter <[e-mail]>
>> Cc: Dave Airlie <[e-mail]>
>> ---
>>  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)
> Probably more typical way to split the lines would be:
> static enum drm_mode_status
> drm_mode_validate_connector_crtc(struct drm_connector *connector,
> 				 struct drm_display_mode *mode)

Agree.

>
> Also 'mode' should be const. Looks like the connector->mode_valid()
> hook is missing the const as well, so that too should be fixed.

Yeah, thats why I didn't use const here. If I change
connector->mode_valid() then I will need to change every driver
who uses it. Please don't get me wrong but I'm not fully
allocated to patch submission, can we focus on getting this patch
merged first and then change the connector->mode_valid() to const
when I have the time?

>
> Not sure about the name of the function. It doesn't really reflect what
> it does in the best way possible. drm_mode_validate_connector_possible_crtcs()
> perhaps? But that is getting quite long so not sure if it' a good idea
> either. I guess simply making the name refer to plural crtcs might be
> a reasonable compromise, ie. drm_mode_validate_connector_crtcs()?

Sounds fine by me :)

>
>> +{
>> +	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;
> A lot of these can be moved into tighter scope.

Agree.

>
>> +	bool callback_found = false;
> I don't think you need this variable at all. See below.
>
>> +	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;
> The need for a comment here tells me that it's probably better
> to got for a more straightforward code. Something like:
>
> mode_status = crtc_funcs->mode_valid(crtc, mode);
> if (mode_statys == MODE_OK)
> 	return mode_status;

Actually I was planning to send a new version with "return" in
the inner loop to avoid unneeded work as the mode would be
accepted anyway, so thats more than agreed :)

>
> And at the end of the function just return MODE_ERROR, or
> some other error value if we have something suitable. Hmm.
> Perhaps we should just return the error from the first or last
> crtc? Either should be pretty easy, just "ret = mode_status" within
> the loop if it didn't return MODE_OK and then 'return ret' at the
> end of the function.

When I was writing this I also though about what error should be
returned on failure. I left the "and" mask but maybe MODE_ERROR
would be more suitable as its more general. I'm thinking that if
we return MODE_SOMETHING then user can be "tricked" because not
all crtcs can agree on the error.

>
>> +		}
>> +	}
>> +
>> +	/* 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);
> Indentation.
>
> And actually, maybe you should also move the connector_funcs->mode_valid()
> call into the new function, and then you could just call the new
> function drm_mode_validate_connector() or something along those lines.

"drm_mode_validate_pipeline" ? i.e. maybe if in the future we
have the need for an encoder->mode_valid? (remote thought)

>
>>  	}
>>  
>>  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
>> +	 * 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);
> Const. Indentation looks off again.

Agree.

>
>> +
>> +	/**
>>  	 * @mode_fixup:
>>  	 *
>>  	 * This callback is used to validate a mode. The parameter mode is the
>> -- 
>> 1.9.1
>>


Best regards,
Jose Miguel Abreu

Ville Syrjälä April 28, 2017, 12:58 p.m. UTC | #5

On Fri, Apr 28, 2017 at 01:30:16PM +0100, Jose Abreu wrote:
> Hi Ville,
> 
> 
> Thanks for the review! My comments inline.
> 
> 
> On 28-04-2017 12:41, Ville Syrjälä 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 <[e-mail]>
> >> Cc: Carlos Palminha <[e-mail]>
> >> Cc: Alexey Brodkin <[e-mail]>
> >> Cc: Ville Syrjälä <[e-mail]>
> >> Cc: Daniel Vetter <[e-mail]>
> >> Cc: Dave Airlie <[e-mail]>
> >> ---
> >>  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)
> > Probably more typical way to split the lines would be:
> > static enum drm_mode_status
> > drm_mode_validate_connector_crtc(struct drm_connector *connector,
> > 				 struct drm_display_mode *mode)
> 
> Agree.
> 
> >
> > Also 'mode' should be const. Looks like the connector->mode_valid()
> > hook is missing the const as well, so that too should be fixed.
> 
> Yeah, thats why I didn't use const here. If I change
> connector->mode_valid() then I will need to change every driver
> who uses it. Please don't get me wrong but I'm not fully
> allocated to patch submission, can we focus on getting this patch
> merged first and then change the connector->mode_valid() to const
> when I have the time?

Should be a good coccinelle exercise ;) But it can certainly be done
later.

> 
> >
> > Not sure about the name of the function. It doesn't really reflect what
> > it does in the best way possible. drm_mode_validate_connector_possible_crtcs()
> > perhaps? But that is getting quite long so not sure if it' a good idea
> > either. I guess simply making the name refer to plural crtcs might be
> > a reasonable compromise, ie. drm_mode_validate_connector_crtcs()?
> 
> Sounds fine by me :)
> 
> >
> >> +{
> >> +	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;
> > A lot of these can be moved into tighter scope.
> 
> Agree.
> 
> >
> >> +	bool callback_found = false;
> > I don't think you need this variable at all. See below.
> >
> >> +	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;
> > The need for a comment here tells me that it's probably better
> > to got for a more straightforward code. Something like:
> >
> > mode_status = crtc_funcs->mode_valid(crtc, mode);
> > if (mode_statys == MODE_OK)
> > 	return mode_status;
> 
> Actually I was planning to send a new version with "return" in
> the inner loop to avoid unneeded work as the mode would be
> accepted anyway, so thats more than agreed :)
> 
> >
> > And at the end of the function just return MODE_ERROR, or
> > some other error value if we have something suitable. Hmm.
> > Perhaps we should just return the error from the first or last
> > crtc? Either should be pretty easy, just "ret = mode_status" within
> > the loop if it didn't return MODE_OK and then 'return ret' at the
> > end of the function.
> 
> When I was writing this I also though about what error should be
> returned on failure. I left the "and" mask but maybe MODE_ERROR
> would be more suitable as its more general. I'm thinking that if
> we return MODE_SOMETHING then user can be "tricked" because not
> all crtcs can agree on the error.

Well it would mean that we would at least give information on what
one crtc though the error was. Otherwise we don't give any
information why the mode was rejected. I'm thinking some infomation is
better than none.

> 
> >
> >> +		}
> >> +	}
> >> +
> >> +	/* 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);
> > Indentation.
> >
> > And actually, maybe you should also move the connector_funcs->mode_valid()
> > call into the new function, and then you could just call the new
> > function drm_mode_validate_connector() or something along those lines.
> 
> "drm_mode_validate_pipeline" ? i.e. maybe if in the future we
> have the need for an encoder->mode_valid? (remote thought)

Well, we pass connector as the thing that drives the validation here, so
having connector is the name is appropriate I think.

> 
> >
> >>  	}
> >>  
> >>  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
> >> +	 * 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);
> > Const. Indentation looks off again.
> 
> Agree.
> 
> >
> >> +
> >> +	/**
> >>  	 * @mode_fixup:
> >>  	 *
> >>  	 * This callback is used to validate a mode. The parameter mode is the
> >> -- 
> >> 1.9.1
> >>
> 
> 
> Best regards,
> Jose Miguel Abreu

Daniel Vetter May 2, 2017, 8:48 a.m. UTC | #6

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 <[e-mail]>
> Cc: Carlos Palminha <[e-mail]>
> Cc: Alexey Brodkin <[e-mail]>
> Cc: Ville Syrjälä <[e-mail]>
> Cc: Daniel Vetter <[e-mail]>
> Cc: Dave Airlie <[e-mail]>

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.

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.
-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
> 
>

Jose Abreu May 2, 2017, 9:29 a.m. UTC | #7

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 <[e-mail]>
>> Cc: Carlos Palminha <[e-mail]>
>> Cc: Alexey Brodkin <[e-mail]>
>> Cc: Ville Syrjälä <[e-mail]>
>> Cc: Daniel Vetter <[e-mail]>
>> Cc: Dave Airlie <[e-mail]>
> 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
>>
>>

Daniel Vetter May 3, 2017, 6:19 a.m. UTC | #8

On Tue, May 2, 2017 at 11:29 AM, Jose Abreu <[e-mail]> wrote:
> 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 <[e-mail]>
>>> Cc: Carlos Palminha <[e-mail]>
>>> Cc: Alexey Brodkin <[e-mail]>
>>> Cc: Ville Syrjälä <[e-mail]>
>>> Cc: Daniel Vetter <[e-mail]>
>>> Cc: Dave Airlie <[e-mail]>
>> 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.

Yes, you are supposed to filter these out in ->mode_valid. But my
stance is that only adding a half-baked support for a new callback to
the core isn't going to make life easier for drivers, it will just add
to the confusion. There's already piles of docs for both @mode_valid
and @mode_fixup hooks explaining this, I don't want to make the
documentation even more complex. And half-baked crtc checking is
_much_ easier to implement in the driver directly (e.g. i915 checks
for crtc constraints since forever, as do the other big x86 drivers).

So all taken together, if we add a ->mode_valid to crtcs, then imo we
should do it right and actually make life easier for drivers. A good
proof would be if your patch would allow us to drop a lot of the
lenghty language from the @mode_valid hooks.
-Daniel

Daniel Vetter May 3, 2017, 6:19 a.m. UTC | #9

On Tue, May 2, 2017 at 11:29 AM, Jose Abreu <[e-mail]> wrote:
> 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 <[e-mail]>
>>> Cc: Carlos Palminha <[e-mail]>
>>> Cc: Alexey Brodkin <[e-mail]>
>>> Cc: Ville Syrjälä <[e-mail]>
>>> Cc: Daniel Vetter <[e-mail]>
>>> Cc: Dave Airlie <[e-mail]>
>> 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.

Yes, you are supposed to filter these out in ->mode_valid. But my
stance is that only adding a half-baked support for a new callback to
the core isn't going to make life easier for drivers, it will just add
to the confusion. There's already piles of docs for both @mode_valid
and @mode_fixup hooks explaining this, I don't want to make the
documentation even more complex. And half-baked crtc checking is
_much_ easier to implement in the driver directly (e.g. i915 checks
for crtc constraints since forever, as do the other big x86 drivers).

So all taken together, if we add a ->mode_valid to crtcs, then imo we
should do it right and actually make life easier for drivers. A good
proof would be if your patch would allow us to drop a lot of the
lenghty language from the @mode_valid hooks.
-Daniel

Jose Abreu May 3, 2017, 2:16 p.m. UTC | #10

Hi Daniel,


On 03-05-2017 07:19, Daniel Vetter wrote:
> On Tue, May 2, 2017 at 11:29 AM, Jose Abreu <[e-mail]> wrote:
>> 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 <[e-mail]>
>>>> Cc: Carlos Palminha <[e-mail]>
>>>> Cc: Alexey Brodkin <[e-mail]>
>>>> Cc: Ville Syrjälä <[e-mail]>
>>>> Cc: Daniel Vetter <[e-mail]>
>>>> Cc: Dave Airlie <[e-mail]>
>>> 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.
> Yes, you are supposed to filter these out in ->mode_valid. But my
> stance is that only adding a half-baked support for a new callback to
> the core isn't going to make life easier for drivers, it will just add
> to the confusion. There's already piles of docs for both @mode_valid
> and @mode_fixup hooks explaining this, I don't want to make the
> documentation even more complex. And half-baked crtc checking is
> _much_ easier to implement in the driver directly (e.g. i915 checks
> for crtc constraints since forever, as do the other big x86 drivers).

But i915 crtc checks are done after handing the mode to
userspace, arcpgu also does that. We must let users specify
manually a mode but there is no point in returning modes in
get_connector which will always fail to commit. I get your point
and this can lead to code duplication, but I don't think it will
lead to confusion as long as it is well documented. And besides,
the callback is completely optional.

>
> So all taken together, if we add a ->mode_valid to crtcs, then imo we
> should do it right and actually make life easier for drivers. A good
> proof would be if your patch would allow us to drop a lot of the
> lenghty language from the @mode_valid hooks.

I completely agree that it should make life easier for drivers
but unfortunately I don't really see how :/

So, in summary:
    Disadvantage 1: Code duplication
    Disadvantage 2: Confusing documentation can lead to callback
misuse

    Advantage 1: User will get life simpler

Best regards,
Jose Miguel Abreu

> -Daniel

Daniel Vetter May 3, 2017, 3 p.m. UTC | #11

On Wed, May 03, 2017 at 03:16:13PM +0100, Jose Abreu wrote:
> Hi Daniel,
> 
> 
> On 03-05-2017 07:19, Daniel Vetter wrote:
> > On Tue, May 2, 2017 at 11:29 AM, Jose Abreu <[e-mail]> wrote:
> >> 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 <[e-mail]>
> >>>> Cc: Carlos Palminha <[e-mail]>
> >>>> Cc: Alexey Brodkin <[e-mail]>
> >>>> Cc: Ville Syrjälä <[e-mail]>
> >>>> Cc: Daniel Vetter <[e-mail]>
> >>>> Cc: Dave Airlie <[e-mail]>
> >>> 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.
> > Yes, you are supposed to filter these out in ->mode_valid. But my
> > stance is that only adding a half-baked support for a new callback to
> > the core isn't going to make life easier for drivers, it will just add
> > to the confusion. There's already piles of docs for both @mode_valid
> > and @mode_fixup hooks explaining this, I don't want to make the
> > documentation even more complex. And half-baked crtc checking is
> > _much_ easier to implement in the driver directly (e.g. i915 checks
> > for crtc constraints since forever, as do the other big x86 drivers).
> 
> But i915 crtc checks are done after handing the mode to
> userspace, arcpgu also does that. We must let users specify
> manually a mode but there is no point in returning modes in
> get_connector which will always fail to commit. I get your point
> and this can lead to code duplication, but I don't think it will
> lead to confusion as long as it is well documented. And besides,
> the callback is completely optional.

Look closer, e.g. intel_dp_mode_valid calls
intel_dp_downstream_max_dotclock which also looks at
dev_priv->max_dotclkc_freq (which is the source dotclk limit, yeah it's a
misnamed function).

And the max dotclk is very much a crtc limit, not a port limit. Note that
a bunch of other ports have port limits which are guaranteed to be lower
than the crtc limit, hence the absence of the checks.

> > So all taken together, if we add a ->mode_valid to crtcs, then imo we
> > should do it right and actually make life easier for drivers. A good
> > proof would be if your patch would allow us to drop a lot of the
> > lenghty language from the @mode_valid hooks.
> 
> I completely agree that it should make life easier for drivers
> but unfortunately I don't really see how :/
> 
> So, in summary:
>     Disadvantage 1: Code duplication
>     Disadvantage 2: Confusing documentation can lead to callback
> misuse
> 
>     Advantage 1: User will get life simpler

Ok, let me try to explain a bit in more detail what I think would be a
real improvement:
- Add ->mode_valid checks to all the places where we currently have
  ->mode_fixup. That'd be crtc, encoder and bridges.

- Pimp the probe helper code to go through all of the combinations,
  filtering out those that aren't allowed by possible_* masks (essentially
  do the same thing that userspace is supposed to do).

- Call all these ->mode_valid checks from the atomic check functions (I
  think we can forget about the legacy crtc helpers for old drivers). Do
  this also for connector->mode_valid.

Taken all together this gives us the guarantee that that any mode which
fails the check in the probe path is guaranteed to never pass in an atomic
commit. And since the probed mode list is what developers generally see,
that's hopefully enough to make sure the filtering is correct.

It is a bit more code than what you've typed here, but not a lot:
- probe path needs to loop over all CRTCxEncoder combos (the
  encoder->bridge routing is fixed) instead over just CRTCs.
- Call ->mode_valid in all the places we already call ->mode_fixup. You
  don't need a new loop over all connectors to be able to call
  ->mode_valid since we already have that connector loop in
  check_modesets().

With that we should also be able to simplify the documentation and rip out
all the warnings about how this is tricky.
-Daniel

Ville Syrjälä May 3, 2017, 3:21 p.m. UTC | #12

On Wed, May 03, 2017 at 05:00:31PM +0200, Daniel Vetter wrote:
> On Wed, May 03, 2017 at 03:16:13PM +0100, Jose Abreu wrote:
> > Hi Daniel,
> > 
> > 
> > On 03-05-2017 07:19, Daniel Vetter wrote:
> > > On Tue, May 2, 2017 at 11:29 AM, Jose Abreu <[e-mail]> wrote:
> > >> 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 <[e-mail]>
> > >>>> Cc: Carlos Palminha <[e-mail]>
> > >>>> Cc: Alexey Brodkin <[e-mail]>
> > >>>> Cc: Ville Syrjälä <[e-mail]>
> > >>>> Cc: Daniel Vetter <[e-mail]>
> > >>>> Cc: Dave Airlie <[e-mail]>
> > >>> 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.
> > > Yes, you are supposed to filter these out in ->mode_valid. But my
> > > stance is that only adding a half-baked support for a new callback to
> > > the core isn't going to make life easier for drivers, it will just add
> > > to the confusion. There's already piles of docs for both @mode_valid
> > > and @mode_fixup hooks explaining this, I don't want to make the
> > > documentation even more complex. And half-baked crtc checking is
> > > _much_ easier to implement in the driver directly (e.g. i915 checks
> > > for crtc constraints since forever, as do the other big x86 drivers).
> > 
> > But i915 crtc checks are done after handing the mode to
> > userspace, arcpgu also does that. We must let users specify
> > manually a mode but there is no point in returning modes in
> > get_connector which will always fail to commit. I get your point
> > and this can lead to code duplication, but I don't think it will
> > lead to confusion as long as it is well documented. And besides,
> > the callback is completely optional.
> 
> Look closer, e.g. intel_dp_mode_valid calls
> intel_dp_downstream_max_dotclock which also looks at
> dev_priv->max_dotclkc_freq (which is the source dotclk limit, yeah it's a
> misnamed function).
> 
> And the max dotclk is very much a crtc limit, not a port limit. Note that
> a bunch of other ports have port limits which are guaranteed to be lower
> than the crtc limit, hence the absence of the checks.
> 
> > > So all taken together, if we add a ->mode_valid to crtcs, then imo we
> > > should do it right and actually make life easier for drivers. A good
> > > proof would be if your patch would allow us to drop a lot of the
> > > lenghty language from the @mode_valid hooks.
> > 
> > I completely agree that it should make life easier for drivers
> > but unfortunately I don't really see how :/
> > 
> > So, in summary:
> >     Disadvantage 1: Code duplication
> >     Disadvantage 2: Confusing documentation can lead to callback
> > misuse
> > 
> >     Advantage 1: User will get life simpler
> 
> Ok, let me try to explain a bit in more detail what I think would be a
> real improvement:
> - Add ->mode_valid checks to all the places where we currently have
>   ->mode_fixup. That'd be crtc, encoder and bridges.
> 
> - Pimp the probe helper code to go through all of the combinations,
>   filtering out those that aren't allowed by possible_* masks (essentially
>   do the same thing that userspace is supposed to do).
> 
> - Call all these ->mode_valid checks from the atomic check functions (I
>   think we can forget about the legacy crtc helpers for old drivers). Do
>   this also for connector->mode_valid.
> 
> Taken all together this gives us the guarantee that that any mode which
> fails the check in the probe path is guaranteed to never pass in an atomic
> commit.

We don't actually want the codepaths to match exactly. In i915
we allow the user to exceed some of the display/dongle limits
because those things often tell us that something shouldn't work
when in fact it does. And some users are quick to complain if
something stops working for them.

Jose Abreu May 4, 2017, 10:21 a.m. UTC | #13

Hi Daniel,


On 03-05-2017 16:00, Daniel Vetter wrote:
> On Wed, May 03, 2017 at 03:16:13PM +0100, Jose Abreu wrote:
>> Hi Daniel,
>>
>>
>> On 03-05-2017 07:19, Daniel Vetter wrote:
>>> On Tue, May 2, 2017 at 11:29 AM, Jose Abreu <[e-mail]> wrote:
>>>> 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 <[e-mail]>
>>>>>> Cc: Carlos Palminha <[e-mail]>
>>>>>> Cc: Alexey Brodkin <[e-mail]>
>>>>>> Cc: Ville Syrjälä <[e-mail]>
>>>>>> Cc: Daniel Vetter <[e-mail]>
>>>>>> Cc: Dave Airlie <[e-mail]>
>>>>> 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.
>>> Yes, you are supposed to filter these out in ->mode_valid. But my
>>> stance is that only adding a half-baked support for a new callback to
>>> the core isn't going to make life easier for drivers, it will just add
>>> to the confusion. There's already piles of docs for both @mode_valid
>>> and @mode_fixup hooks explaining this, I don't want to make the
>>> documentation even more complex. And half-baked crtc checking is
>>> _much_ easier to implement in the driver directly (e.g. i915 checks
>>> for crtc constraints since forever, as do the other big x86 drivers).
>> But i915 crtc checks are done after handing the mode to
>> userspace, arcpgu also does that. We must let users specify
>> manually a mode but there is no point in returning modes in
>> get_connector which will always fail to commit. I get your point
>> and this can lead to code duplication, but I don't think it will
>> lead to confusion as long as it is well documented. And besides,
>> the callback is completely optional.
> Look closer, e.g. intel_dp_mode_valid calls
> intel_dp_downstream_max_dotclock which also looks at
> dev_priv->max_dotclkc_freq (which is the source dotclk limit, yeah it's a
> misnamed function).
>
> And the max dotclk is very much a crtc limit, not a port limit. Note that
> a bunch of other ports have port limits which are guaranteed to be lower
> than the crtc limit, hence the absence of the checks.
>
>>> So all taken together, if we add a ->mode_valid to crtcs, then imo we
>>> should do it right and actually make life easier for drivers. A good
>>> proof would be if your patch would allow us to drop a lot of the
>>> lenghty language from the @mode_valid hooks.
>> I completely agree that it should make life easier for drivers
>> but unfortunately I don't really see how :/
>>
>> So, in summary:
>>     Disadvantage 1: Code duplication
>>     Disadvantage 2: Confusing documentation can lead to callback
>> misuse
>>
>>     Advantage 1: User will get life simpler
> Ok, let me try to explain a bit in more detail what I think would be a
> real improvement:
> - Add ->mode_valid checks to all the places where we currently have
>   ->mode_fixup. That'd be crtc, encoder and bridges.
>
> - Pimp the probe helper code to go through all of the combinations,
>   filtering out those that aren't allowed by possible_* masks (essentially
>   do the same thing that userspace is supposed to do).
>
> - Call all these ->mode_valid checks from the atomic check functions (I
>   think we can forget about the legacy crtc helpers for old drivers). Do
>   this also for connector->mode_valid.
>
> Taken all together this gives us the guarantee that that any mode which
> fails the check in the probe path is guaranteed to never pass in an atomic
> commit. And since the probed mode list is what developers generally see,
> that's hopefully enough to make sure the filtering is correct.
>
> It is a bit more code than what you've typed here, but not a lot:
> - probe path needs to loop over all CRTCxEncoder combos (the
>   encoder->bridge routing is fixed) instead over just CRTCs.
> - Call ->mode_valid in all the places we already call ->mode_fixup. You
>   don't need a new loop over all connectors to be able to call
>   ->mode_valid since we already have that connector loop in
>   check_modesets().
>
> With that we should also be able to simplify the documentation and rip out
> all the warnings about how this is tricky.

This seems very nice! So we essentially can remove the validation
of modes in atomic_check as mode_valid will be called before, right?

Best regards,
Jose Miguel Abreu

> -Daniel

Jose Abreu May 4, 2017, 11:55 a.m. UTC | #14

Hi Daniel,


On 04-05-2017 11:21, Jose Abreu wrote:
> Hi Daniel,
>
>
> On 03-05-2017 16:00, Daniel Vetter wrote:
>> On Wed, May 03, 2017 at 03:16:13PM +0100, Jose Abreu wrote:
>>> Hi Daniel,
>>>
>>>
>>> On 03-05-2017 07:19, Daniel Vetter wrote:
>>>> On Tue, May 2, 2017 at 11:29 AM, Jose Abreu <[e-mail]> wrote:
>>>>> 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 <[e-mail]>
>>>>>>> Cc: Carlos Palminha <[e-mail]>
>>>>>>> Cc: Alexey Brodkin <[e-mail]>
>>>>>>> Cc: Ville Syrjälä <[e-mail]>
>>>>>>> Cc: Daniel Vetter <[e-mail]>
>>>>>>> Cc: Dave Airlie <[e-mail]>
>>>>>> 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.
>>>> Yes, you are supposed to filter these out in ->mode_valid. But my
>>>> stance is that only adding a half-baked support for a new callback to
>>>> the core isn't going to make life easier for drivers, it will just add
>>>> to the confusion. There's already piles of docs for both @mode_valid
>>>> and @mode_fixup hooks explaining this, I don't want to make the
>>>> documentation even more complex. And half-baked crtc checking is
>>>> _much_ easier to implement in the driver directly (e.g. i915 checks
>>>> for crtc constraints since forever, as do the other big x86 drivers).
>>> But i915 crtc checks are done after handing the mode to
>>> userspace, arcpgu also does that. We must let users specify
>>> manually a mode but there is no point in returning modes in
>>> get_connector which will always fail to commit. I get your point
>>> and this can lead to code duplication, but I don't think it will
>>> lead to confusion as long as it is well documented. And besides,
>>> the callback is completely optional.
>> Look closer, e.g. intel_dp_mode_valid calls
>> intel_dp_downstream_max_dotclock which also looks at
>> dev_priv->max_dotclkc_freq (which is the source dotclk limit, yeah it's a
>> misnamed function).
>>
>> And the max dotclk is very much a crtc limit, not a port limit. Note that
>> a bunch of other ports have port limits which are guaranteed to be lower
>> than the crtc limit, hence the absence of the checks.
>>
>>>> So all taken together, if we add a ->mode_valid to crtcs, then imo we
>>>> should do it right and actually make life easier for drivers. A good
>>>> proof would be if your patch would allow us to drop a lot of the
>>>> lenghty language from the @mode_valid hooks.
>>> I completely agree that it should make life easier for drivers
>>> but unfortunately I don't really see how :/
>>>
>>> So, in summary:
>>>     Disadvantage 1: Code duplication
>>>     Disadvantage 2: Confusing documentation can lead to callback
>>> misuse
>>>
>>>     Advantage 1: User will get life simpler
>> Ok, let me try to explain a bit in more detail what I think would be a
>> real improvement:
>> - Add ->mode_valid checks to all the places where we currently have
>>   ->mode_fixup. That'd be crtc, encoder and bridges.
>>
>> - Pimp the probe helper code to go through all of the combinations,
>>   filtering out those that aren't allowed by possible_* masks (essentially
>>   do the same thing that userspace is supposed to do).
>>
>> - Call all these ->mode_valid checks from the atomic check functions (I
>>   think we can forget about the legacy crtc helpers for old drivers). Do
>>   this also for connector->mode_valid.
>>
>> Taken all together this gives us the guarantee that that any mode which
>> fails the check in the probe path is guaranteed to never pass in an atomic
>> commit. And since the probed mode list is what developers generally see,
>> that's hopefully enough to make sure the filtering is correct.
>>
>> It is a bit more code than what you've typed here, but not a lot:
>> - probe path needs to loop over all CRTCxEncoder combos (the
>>   encoder->bridge routing is fixed) instead over just CRTCs.
>> - Call ->mode_valid in all the places we already call ->mode_fixup. You
>>   don't need a new loop over all connectors to be able to call
>>   ->mode_valid since we already have that connector loop in
>>   check_modesets().
>>
>> With that we should also be able to simplify the documentation and rip out
>> all the warnings about how this is tricky.
> This seems very nice! So we essentially can remove the validation
> of modes in atomic_check as mode_valid will be called before, right?
>
> Best regards,
> Jose Miguel Abreu

One more thing: When should the mode_valid callback be called?
Before or after mode_fixup/atomic_check? I think it makes sense
to call it after, so that if a fixup to the mode is needed then
we call mode_valid() after with the adjusted mode. What do you think?

Best regards,
Jose Miguel Abreu

>
>> -Daniel

Daniel Vetter May 4, 2017, 12:48 p.m. UTC | #15

On Thu, May 4, 2017 at 1:55 PM, Jose Abreu <[e-mail]> wrote:
> On 04-05-2017 11:21, Jose Abreu wrote:
>> Hi Daniel,
>>
>>
>> On 03-05-2017 16:00, Daniel Vetter wrote:
>>> On Wed, May 03, 2017 at 03:16:13PM +0100, Jose Abreu wrote:
>>>> Hi Daniel,
>>>>
>>>>
>>>> On 03-05-2017 07:19, Daniel Vetter wrote:
>>>>> On Tue, May 2, 2017 at 11:29 AM, Jose Abreu <[e-mail]> wrote:
>>>>>> 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 <[e-mail]>
>>>>>>>> Cc: Carlos Palminha <[e-mail]>
>>>>>>>> Cc: Alexey Brodkin <[e-mail]>
>>>>>>>> Cc: Ville Syrjälä <[e-mail]>
>>>>>>>> Cc: Daniel Vetter <[e-mail]>
>>>>>>>> Cc: Dave Airlie <[e-mail]>
>>>>>>> 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.
>>>>> Yes, you are supposed to filter these out in ->mode_valid. But my
>>>>> stance is that only adding a half-baked support for a new callback to
>>>>> the core isn't going to make life easier for drivers, it will just add
>>>>> to the confusion. There's already piles of docs for both @mode_valid
>>>>> and @mode_fixup hooks explaining this, I don't want to make the
>>>>> documentation even more complex. And half-baked crtc checking is
>>>>> _much_ easier to implement in the driver directly (e.g. i915 checks
>>>>> for crtc constraints since forever, as do the other big x86 drivers).
>>>> But i915 crtc checks are done after handing the mode to
>>>> userspace, arcpgu also does that. We must let users specify
>>>> manually a mode but there is no point in returning modes in
>>>> get_connector which will always fail to commit. I get your point
>>>> and this can lead to code duplication, but I don't think it will
>>>> lead to confusion as long as it is well documented. And besides,
>>>> the callback is completely optional.
>>> Look closer, e.g. intel_dp_mode_valid calls
>>> intel_dp_downstream_max_dotclock which also looks at
>>> dev_priv->max_dotclkc_freq (which is the source dotclk limit, yeah it's a
>>> misnamed function).
>>>
>>> And the max dotclk is very much a crtc limit, not a port limit. Note that
>>> a bunch of other ports have port limits which are guaranteed to be lower
>>> than the crtc limit, hence the absence of the checks.
>>>
>>>>> So all taken together, if we add a ->mode_valid to crtcs, then imo we
>>>>> should do it right and actually make life easier for drivers. A good
>>>>> proof would be if your patch would allow us to drop a lot of the
>>>>> lenghty language from the @mode_valid hooks.
>>>> I completely agree that it should make life easier for drivers
>>>> but unfortunately I don't really see how :/
>>>>
>>>> So, in summary:
>>>>     Disadvantage 1: Code duplication
>>>>     Disadvantage 2: Confusing documentation can lead to callback
>>>> misuse
>>>>
>>>>     Advantage 1: User will get life simpler
>>> Ok, let me try to explain a bit in more detail what I think would be a
>>> real improvement:
>>> - Add ->mode_valid checks to all the places where we currently have
>>>   ->mode_fixup. That'd be crtc, encoder and bridges.
>>>
>>> - Pimp the probe helper code to go through all of the combinations,
>>>   filtering out those that aren't allowed by possible_* masks (essentially
>>>   do the same thing that userspace is supposed to do).
>>>
>>> - Call all these ->mode_valid checks from the atomic check functions (I
>>>   think we can forget about the legacy crtc helpers for old drivers). Do
>>>   this also for connector->mode_valid.
>>>
>>> Taken all together this gives us the guarantee that that any mode which
>>> fails the check in the probe path is guaranteed to never pass in an atomic
>>> commit. And since the probed mode list is what developers generally see,
>>> that's hopefully enough to make sure the filtering is correct.
>>>
>>> It is a bit more code than what you've typed here, but not a lot:
>>> - probe path needs to loop over all CRTCxEncoder combos (the
>>>   encoder->bridge routing is fixed) instead over just CRTCs.
>>> - Call ->mode_valid in all the places we already call ->mode_fixup. You
>>>   don't need a new loop over all connectors to be able to call
>>>   ->mode_valid since we already have that connector loop in
>>>   check_modesets().
>>>
>>> With that we should also be able to simplify the documentation and rip out
>>> all the warnings about how this is tricky.
>> This seems very nice! So we essentially can remove the validation
>> of modes in atomic_check as mode_valid will be called before, right?
>>
>> Best regards,
>> Jose Miguel Abreu
>
> One more thing: When should the mode_valid callback be called?
> Before or after mode_fixup/atomic_check? I think it makes sense
> to call it after, so that if a fixup to the mode is needed then
> we call mode_valid() after with the adjusted mode. What do you think?

I'd call it before, because if you call it afterwards then you won't
correctly filter things in the probe path (since those don't adjust
the mode). We can't make this perfect, but since it's a helper, 90% is
good enough.
-Daniel

Daniel Vetter May 4, 2017, 12:49 p.m. UTC | #16

On Wed, May 3, 2017 at 5:21 PM, Ville Syrjälä
<[e-mail]> wrote:
> We don't actually want the codepaths to match exactly. In i915
> we allow the user to exceed some of the display/dongle limits
> because those things often tell us that something shouldn't work
> when in fact it does. And some users are quick to complain if
> something stops working for them.

The goal here is to share the source-side checking
(crtc/encoder/bridges), and that should match perfectly between probe
and commit. Sink-side constraints are different, and for those we
should indeed not check everything. Maybe a good reason to only call
connector->mode_valid in the probe paths?
-Daniel

Ville Syrjälä May 4, 2017, 1:08 p.m. UTC | #17

On Thu, May 04, 2017 at 02:49:31PM +0200, Daniel Vetter wrote:
> On Wed, May 3, 2017 at 5:21 PM, Ville Syrjälä
> <[e-mail]> wrote:
> > We don't actually want the codepaths to match exactly. In i915
> > we allow the user to exceed some of the display/dongle limits
> > because those things often tell us that something shouldn't work
> > when in fact it does. And some users are quick to complain if
> > something stops working for them.
> 
> The goal here is to share the source-side checking
> (crtc/encoder/bridges), and that should match perfectly between probe
> and commit. Sink-side constraints are different, and for those we
> should indeed not check everything. Maybe a good reason to only call
> connector->mode_valid in the probe paths?

I thought you wanted to call it from both. But I guess if we
have a .mode_valid() hook on the encoder as well to handle
the source port limits then it could work out.

Ddi encoders might cause a problem though since we have to know
whether we're going to do DP or HDMI before we check the limits,
so it would need to be done after we've figured out output_types
or else we'll need to duplicate some logic to determine which
case we're dealing with.

Patch

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
+	 * 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