Message ID | 20200211162208.16224-8-ville.syrjala@linux.intel.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | drm: Try to fix encoder possible_clones/crtc | expand |
On Tue, Feb 11, 2020 at 06:22:08PM +0200, Ville Syrjala wrote: > From: Ville Syrjälä <ville.syrjala@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@suse.de> > Cc: Daniel Vetter <daniel@ffwll.ch> > Signed-off-by: Ville Syrjälä <ville.syrjala@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. Reviewed-by: Daniel Vetter <daniel.vetter@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 >
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@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@suse.de> > > Cc: Daniel Vetter <daniel@ffwll.ch> > > Signed-off-by: Ville Syrjälä <ville.syrjala@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. > > Reviewed-by: Daniel Vetter <daniel.vetter@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
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@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@suse.de> > > > Cc: Daniel Vetter <daniel@ffwll.ch> > > > Signed-off-by: Ville Syrjälä <ville.syrjala@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@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
On Wed, Feb 12, 2020 at 10:07:55AM +0100, Daniel Vetter wrote: > 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@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@suse.de> > > > > Cc: Daniel Vetter <daniel@ffwll.ch> > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@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. Strike that, it sets it using tegra_output_find_possible_crtcs(). I think everything is good and we really don't need this patch here to fix up possible_crtcs. -Daniel > > 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@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
On Wed, Feb 12, 2020 at 10:08:49AM +0100, Daniel Vetter wrote: > On Wed, Feb 12, 2020 at 10:07:55AM +0100, Daniel Vetter wrote: > > 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@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@suse.de> > > > > > Cc: Daniel Vetter <daniel@ffwll.ch> > > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@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. > > Strike that, it sets it using tegra_output_find_possible_crtcs(). > > I think everything is good and we really don't need this patch here to fix > up possible_crtcs. Finally pushed the other patches from the series to drm-misc-next. Thanks for the reviews. Should the new possible_{crtcs,clones} WARNs start to trigger for anyone despite our best efforts, please holler and I'll look into what needs fixing.
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