| Message ID | 1476113170-13816-1-git-send-email-jani.nikula@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Oct 10, 2016 at 06:26:10PM +0300, Jani Nikula wrote: > Signed-off-by: Jani Nikula <jani.nikula@intel.com> > > --- > > I needed this for some stuff that turned out to be a dead end. But this > seems to be the right thing to do anyway. Applied to drm-misc. There's also the connector and plane versions of this, care to spin them too? -Daniel > --- > include/drm/drm_crtc.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h > index 61932f55f788..0aa292526567 100644 > --- a/include/drm/drm_crtc.h > +++ b/include/drm/drm_crtc.h > @@ -1342,7 +1342,7 @@ extern void drm_crtc_cleanup(struct drm_crtc *crtc); > * Given a registered CRTC, return the index of that CRTC within a DRM > * device's list of CRTCs. > */ > -static inline unsigned int drm_crtc_index(struct drm_crtc *crtc) > +static inline unsigned int drm_crtc_index(const struct drm_crtc *crtc) > { > return crtc->index; > } > -- > 2.1.4 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Mon, Oct 10, 2016 at 06:26:10PM +0300, Jani Nikula wrote: > Signed-off-by: Jani Nikula <jani.nikula@intel.com> Plus equivalents in drm_plane.h, drm_encoder.h ... Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> -Chris
On Mon, Oct 10, 2016 at 06:26:10PM +0300, Jani Nikula wrote: > Signed-off-by: Jani Nikula <jani.nikula@intel.com> > > --- > > I needed this for some stuff that turned out to be a dead end. But this > seems to be the right thing to do anyway. > --- > include/drm/drm_crtc.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h > index 61932f55f788..0aa292526567 100644 > --- a/include/drm/drm_crtc.h > +++ b/include/drm/drm_crtc.h > @@ -1342,7 +1342,7 @@ extern void drm_crtc_cleanup(struct drm_crtc *crtc); > * Given a registered CRTC, return the index of that CRTC within a DRM > * device's list of CRTCs. > */ > -static inline unsigned int drm_crtc_index(struct drm_crtc *crtc) > +static inline unsigned int drm_crtc_index(const struct drm_crtc *crtc) > { > return crtc->index; BTW speaking about the index stuff. It dawned on me recently that calling drm_crtc_cleanup() & co. is totally not safe except maybe during the final cleanup. If you would do something like: a = drm_crtc_init(); b = drm_crtc_init(); drm_crtc_cleanup(a); c = drm_crtc_init(); b and c would end up with the same index. Or if you would do just a = drm_crtc_init(); b = drm_crtc_init(); drm_crtc_cleanup(a); We'd end up with num_crtc==1, but b->index==1, so we'd actually access beyond the allocated arrays in a bunch of places. This would need to fixed somehow, or at least documented clearly that if you have to call any of the cleanup functions during init, you have to abort the entire thing.
On Mon, Oct 10, 2016 at 6:04 PM, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > On Mon, Oct 10, 2016 at 06:26:10PM +0300, Jani Nikula wrote: >> Signed-off-by: Jani Nikula <jani.nikula@intel.com> >> >> --- >> >> I needed this for some stuff that turned out to be a dead end. But this >> seems to be the right thing to do anyway. >> --- >> include/drm/drm_crtc.h | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h >> index 61932f55f788..0aa292526567 100644 >> --- a/include/drm/drm_crtc.h >> +++ b/include/drm/drm_crtc.h >> @@ -1342,7 +1342,7 @@ extern void drm_crtc_cleanup(struct drm_crtc *crtc); >> * Given a registered CRTC, return the index of that CRTC within a DRM >> * device's list of CRTCs. >> */ >> -static inline unsigned int drm_crtc_index(struct drm_crtc *crtc) >> +static inline unsigned int drm_crtc_index(const struct drm_crtc *crtc) >> { >> return crtc->index; > > BTW speaking about the index stuff. It dawned on me recently that calling > drm_crtc_cleanup() & co. is totally not safe except maybe during the > final cleanup. > > If you would do something like: > a = drm_crtc_init(); > b = drm_crtc_init(); > drm_crtc_cleanup(a); > c = drm_crtc_init(); > > b and c would end up with the same index. > > > Or if you would do just > a = drm_crtc_init(); > b = drm_crtc_init(); > drm_crtc_cleanup(a); > > We'd end up with num_crtc==1, but b->index==1, so we'd actually access > beyond the allocated arrays in a bunch of places. > > This would need to fixed somehow, or at least documented clearly that if > you have to call any of the cleanup functions during init, you have to > abort the entire thing. You can't hotplug CRTCs, like you can't hotplug anything else than connectors. And for connectors we have an ida to make sure you never end up with duplicated indizes. So I think we're safe. -Daniel
On Mon, Oct 10, 2016 at 06:10:42PM +0200, Daniel Vetter wrote: > On Mon, Oct 10, 2016 at 6:04 PM, Ville Syrjälä > <ville.syrjala@linux.intel.com> wrote: > > On Mon, Oct 10, 2016 at 06:26:10PM +0300, Jani Nikula wrote: > >> Signed-off-by: Jani Nikula <jani.nikula@intel.com> > >> > >> --- > >> > >> I needed this for some stuff that turned out to be a dead end. But this > >> seems to be the right thing to do anyway. > >> --- > >> include/drm/drm_crtc.h | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h > >> index 61932f55f788..0aa292526567 100644 > >> --- a/include/drm/drm_crtc.h > >> +++ b/include/drm/drm_crtc.h > >> @@ -1342,7 +1342,7 @@ extern void drm_crtc_cleanup(struct drm_crtc *crtc); > >> * Given a registered CRTC, return the index of that CRTC within a DRM > >> * device's list of CRTCs. > >> */ > >> -static inline unsigned int drm_crtc_index(struct drm_crtc *crtc) > >> +static inline unsigned int drm_crtc_index(const struct drm_crtc *crtc) > >> { > >> return crtc->index; > > > > BTW speaking about the index stuff. It dawned on me recently that calling > > drm_crtc_cleanup() & co. is totally not safe except maybe during the > > final cleanup. > > > > If you would do something like: > > a = drm_crtc_init(); > > b = drm_crtc_init(); > > drm_crtc_cleanup(a); > > c = drm_crtc_init(); > > > > b and c would end up with the same index. > > > > > > Or if you would do just > > a = drm_crtc_init(); > > b = drm_crtc_init(); > > drm_crtc_cleanup(a); > > > > We'd end up with num_crtc==1, but b->index==1, so we'd actually access > > beyond the allocated arrays in a bunch of places. > > > > This would need to fixed somehow, or at least documented clearly that if > > you have to call any of the cleanup functions during init, you have to > > abort the entire thing. > > You can't hotplug CRTCs, like you can't hotplug anything else than > connectors. And for connectors we have an ida to make sure you never > end up with duplicated indizes. So I think we're safe. Except in i915 we don't abort the driver load if we would fail part way into the crtc init. So we'd just keep going with bogus indexes and whatnot. I think I wrote a patch to change that, but I'd have find it again to be sure. Other drivers may or may not have the same problem.
On Mon, Oct 10, 2016 at 6:30 PM, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: >> You can't hotplug CRTCs, like you can't hotplug anything else than >> connectors. And for connectors we have an ida to make sure you never >> end up with duplicated indizes. So I think we're safe. > > Except in i915 we don't abort the driver load if we would fail part > way into the crtc init. So we'd just keep going with bogus indexes > and whatnot. I think I wrote a patch to change that, but I'd have > find it again to be sure. Other drivers may or may not have the same > problem. Hm, we should at least make sure we don't leave any partially initialized modeset objects behind when the modeset part of the driver fails to load. -Daniel
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 61932f55f788..0aa292526567 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -1342,7 +1342,7 @@ extern void drm_crtc_cleanup(struct drm_crtc *crtc); * Given a registered CRTC, return the index of that CRTC within a DRM * device's list of CRTCs. */ -static inline unsigned int drm_crtc_index(struct drm_crtc *crtc) +static inline unsigned int drm_crtc_index(const struct drm_crtc *crtc) { return crtc->index; }
Signed-off-by: Jani Nikula <jani.nikula@intel.com> --- I needed this for some stuff that turned out to be a dead end. But this seems to be the right thing to do anyway. --- include/drm/drm_crtc.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)