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

[Ville Syrjälä]

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

-- 
Ville Syrjälä
Intel OTC