Re: [PATCH 4/5] drm: Use mode_valid() in atomic modeset

[Daniel Vetter]

On Thu, May 04, 2017 at 04:13:51PM +0100, Jose Abreu wrote:
> On 04-05-2017 15:40, Ville Syrjälä wrote:
> > On Thu, May 04, 2017 at 03:11:41PM +0100, Jose Abreu wrote:
> >> +					    struct drm_encoder *encoder,
> >> +					    struct drm_crtc *crtc,
> >> +					    struct drm_display_mode *mode)
> >> +{
> >> +	const struct drm_crtc_helper_funcs *crtc_funcs = crtc->helper_private;
> >> +	const struct drm_connector_helper_funcs *connector_funcs =
> >> +		connector->helper_private;
> >> +	const struct drm_encoder_helper_funcs *encoder_funcs =
> >> +		encoder->helper_private;
> >> +	enum drm_mode_status ret = MODE_OK;
> >> +
> >> +	if (connector_funcs && connector_funcs->mode_valid)
> >> +		ret = connector_funcs->mode_valid(connector, mode);
> > As I mentioned earlier, this would break i915. We either can't call the 
> > connector hook at all here, or we have to make it somehow opt-in if some
> > drivers really want to do it.
> 
> Ok. You said you let users exceed the limits, but why doesn't it
> fail in atomic_check and will fail in mode_valid? I guess I can
> remove it, but this can lead to lots of confusion, i.e. with this
> series the mode_valid callbacks for all components are called, if
> I remove just one call the whole point will fall apart.
> 
> Can we think about something else ? I think making it opt-in is
> not ideal, if the helper is there then it should be used, but if
> thats the best solution then shall we add a flag which will call
> or not *all* the mode_valid callbacks in this stage
> (drm_device.allow_modevalid_call, ...) ?

This is a general issue, not a driver opt-in. With your new callbacks
ideally we'll have:
- all the source checks (clock limits, size limits) are in the
  crtc/encoder/bridge callbacks
- only the sink limit checks (derived from edid or DP aux) are in the
  connector callback

Letting userspace overwrite these seems like a reasonable idea that we
should support in general I think. See also my comment on patch 1 for the
connector's mode_valid kerneldoc.

For arcpgu that might mean that you need to move part of the connector's
mode_valid checks into the encoder, but since all the encoder modeset is
in there already, that seems like a good cleanup of the drm modeset
framework.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch