[Intel-gfx] [PATCH v3 7/7] drm: Allow drivers to leave encoder->possible_crtcs==0

[Daniel Vetter]

On Tue, Feb 11, 2020 at 07:14:51PM +0200, Ville Syrjälä wrote:
> On Tue, Feb 11, 2020 at 06:05:45PM +0100, Daniel Vetter wrote:
> > On Tue, Feb 11, 2020 at 06:22:08PM +0200, Ville Syrjala wrote:
> > > From: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > > 
> > > Let's simplify life of driver by allowing them to leave
> > > encoder->possible_crtcs unset if they have no restrictions
> > > in crtc<->encoder linkage. We'll just populate possible_crtcs
> > > with the full crtc mask when registering the encoder so that
> > > userspace doesn't have to deal with drivers not populating
> > > this correctly.
> > > 
> > > Cc: Thomas Zimmermann <tzimmermann at suse.de>
> > > Cc: Daniel Vetter <daniel at ffwll.ch>
> > > Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > > ---
> > > We might not actually need/want this, but included it here for
> > > future reference if that assumption turns out to be wrong.
> > 
> > I think this one is most definitely needed. _Lots_ of drivers get this
> > toally wrong and just leave the value blank. It's encoded as official
> > fallback in most userspace compositors.
> 
> OK. It's been a while since I dug around so can't really remmber how
> this was being handled. I'll reorder before pushing.

Hm otoh having "works with all crtcs" as default is a bit dangerous,
whereas the "cannot be cloned" default for possible_clones is perfectly
safe.

So now I'm kinda not sure whether this is a bright idea, and we shouldn't
just eat the cost of fixing up all the various WARNING backtraces your
previous patch triggers. I've done a full review and the following look
suspect:

- tegara/sor.c Strangely it's the only one, the other output drivers do
  seem to set the possible_crtcs mask to something useful.

Everything else seems to be fine. I'd say let's drop this patch, and I'm
adding Thierry to shed some light on what's going on in tegra/sor.c.
-Daniel

> 
> > 
> > Reviewed-by: Daniel Vetter <daniel.vetter at ffwll.ch>
> > 
> > > ---
> > >  drivers/gpu/drm/drm_mode_config.c | 15 ++++++++++++++-
> > >  include/drm/drm_encoder.h         |  4 ++++
> > >  2 files changed, 18 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c
> > > index 4c1b350ddb95..ce18c3dd0bde 100644
> > > --- a/drivers/gpu/drm/drm_mode_config.c
> > > +++ b/drivers/gpu/drm/drm_mode_config.c
> > > @@ -592,6 +592,17 @@ static u32 full_crtc_mask(struct drm_device *dev)
> > >  	return crtc_mask;
> > >  }
> > >  
> > > +/*
> > > + * Make life easy for drivers by allowing them to leave
> > > + * possible_crtcs unset if there are not crtc<->encoder
> > > + * restrictions.
> > > + */
> > > +static void fixup_encoder_possible_crtcs(struct drm_encoder *encoder)
> > > +{
> > > +	if (encoder->possible_crtcs == 0)
> > > +		encoder->possible_crtcs = full_crtc_mask(encoder->dev);
> > > +}
> > > +
> > >  static void validate_encoder_possible_crtcs(struct drm_encoder *encoder)
> > >  {
> > >  	u32 crtc_mask = full_crtc_mask(encoder->dev);
> > > @@ -608,8 +619,10 @@ void drm_mode_config_validate(struct drm_device *dev)
> > >  {
> > >  	struct drm_encoder *encoder;
> > >  
> > > -	drm_for_each_encoder(encoder, dev)
> > > +	drm_for_each_encoder(encoder, dev) {
> > >  		fixup_encoder_possible_clones(encoder);
> > > +		fixup_encoder_possible_crtcs(encoder);
> > > +	}
> > >  
> > >  	drm_for_each_encoder(encoder, dev) {
> > >  		validate_encoder_possible_clones(encoder);
> > > diff --git a/include/drm/drm_encoder.h b/include/drm/drm_encoder.h
> > > index b236269f41ac..bd033c5618bf 100644
> > > --- a/include/drm/drm_encoder.h
> > > +++ b/include/drm/drm_encoder.h
> > > @@ -142,6 +142,10 @@ struct drm_encoder {
> > >  	 * the bits for all &drm_crtc objects this encoder can be connected to
> > >  	 * before calling drm_dev_register().
> > >  	 *
> > > +	 * As an exception to the above rule if any crtc can be connected to
> > > +	 * the encoder the driver can leave @possible_crtcs set to 0. The core
> > > +	 * will automagically fix this up by setting the bit for every crtc.
> > > +	 *
> > >  	 * You will get a WARN if you get this wrong in the driver.
> > >  	 *
> > >  	 * Note that since CRTC objects can't be hotplugged the assigned indices
> > > -- 
> > > 2.24.1
> > > 
> > 
> > -- 
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
> 
> -- 
> Ville Syrjälä
> Intel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch