| Message ID | 1389613610-21084-2-git-send-email-treding@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi On Mon, Jan 13, 2014 at 12:46 PM, Thierry Reding <thierry.reding@gmail.com> wrote: > From: Russell King <rmk+kernel@arm.linux.org.uk> > > The encoder possible_crtcs mask identifies which CRTCs can be bound to > a particular encoder. Each bit from bit 0 defines an index in the list > of CRTCs held in the DRM mode_config crtc_list. Rather than having > drivers trying to track the position of their CRTCs in the list, expose > the code which already exists for calculating the appropriate mask bit > for a CRTC. > > Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk> > Reviewed-by: David Herrmann <dh.herrmann@gmail.com> > [treding@nvidia.com: add drm_crtc_index(), move to core] > Signed-off-by: Thierry Reding <treding@nvidia.com> > --- > Changes in v2: > - further extract a new drm_crtc_index() function > - move to DRM core (drm_crtc.c) > > drivers/gpu/drm/drm_crtc.c | 38 ++++++++++++++++++++++++++++++++++++++ > drivers/gpu/drm/drm_crtc_helper.c | 16 +++------------- > include/drm/drm_crtc.h | 2 ++ > 3 files changed, 43 insertions(+), 13 deletions(-) > > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c > index 266a01d7f635..fdfa5a0e51d6 100644 > --- a/drivers/gpu/drm/drm_crtc.c > +++ b/drivers/gpu/drm/drm_crtc.c > @@ -675,6 +675,44 @@ void drm_crtc_cleanup(struct drm_crtc *crtc) > EXPORT_SYMBOL(drm_crtc_cleanup); > > /** > + * drm_crtc_index - find the index of a registered CRTC > + * @crtc: CRTC to find index for > + * > + * Given a registered CRTC, return the index of that CRTC within a DRM > + * device's list of CRTCs. If the CRTC isn't registered, return -1. > + */ > +int drm_crtc_index(struct drm_crtc *crtc) > +{ > + struct drm_crtc *tmp; > + int index = 0; > + > + list_for_each_entry(tmp, &crtc->dev->mode_config.crtc_list, head) { > + if (tmp == crtc) > + return index; > + > + index++; > + } > + > + return -1; Does that -1 ever make sense? We don't support mode-object-hotplugging so all "drm_crtc" objects are known at initialization time. I'd rather put a BUG() here than a silent -1. This also makes drm_crtc_mask() redundant. Anyhow, still r-b by me. Thanks David > +} > +EXPORT_SYMBOL(drm_crtc_index); > + > +/** > + * drm_crtc_mask - find the mask of a registered CRTC > + * @crtc: CRTC to find mask for > + * > + * Given a registered CRTC, return the mask bit of that CRTC for an > + * encoder's possible_crtcs field. > + */ > +uint32_t drm_crtc_mask(struct drm_crtc *crtc) > +{ > + int i = drm_crtc_index(crtc); > + > + return i < 0 ? 0 : 1 << i; > +} > +EXPORT_SYMBOL(drm_crtc_mask); > + > +/** > * drm_mode_probed_add - add a mode to a connector's probed mode list > * @connector: connector the new mode > * @mode: mode data > diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c > index 01361aba033b..95b32098badf 100644 > --- a/drivers/gpu/drm/drm_crtc_helper.c > +++ b/drivers/gpu/drm/drm_crtc_helper.c > @@ -334,23 +334,13 @@ EXPORT_SYMBOL(drm_helper_disable_unused_functions); > static bool drm_encoder_crtc_ok(struct drm_encoder *encoder, > struct drm_crtc *crtc) > { > - struct drm_device *dev; > - struct drm_crtc *tmp; > - int crtc_mask = 1; > + uint32_t crtc_mask; > > WARN(!crtc, "checking null crtc?\n"); > > - dev = crtc->dev; > - > - list_for_each_entry(tmp, &dev->mode_config.crtc_list, head) { > - if (tmp == crtc) > - break; > - crtc_mask <<= 1; > - } > + crtc_mask = drm_crtc_mask(crtc); > > - if (encoder->possible_crtcs & crtc_mask) > - return true; > - return false; > + return !!(encoder->possible_crtcs & crtc_mask); > } > > /* > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h > index f32c5cd51f41..a61fb73fdcb5 100644 > --- a/include/drm/drm_crtc.h > +++ b/include/drm/drm_crtc.h > @@ -929,6 +929,8 @@ extern int drm_crtc_init(struct drm_device *dev, > struct drm_crtc *crtc, > const struct drm_crtc_funcs *funcs); > extern void drm_crtc_cleanup(struct drm_crtc *crtc); > +extern int drm_crtc_index(struct drm_crtc *crtc); > +extern uint32_t drm_crtc_mask(struct drm_crtc *crtc); > > extern void drm_connector_ida_init(void); > extern void drm_connector_ida_destroy(void); > -- > 1.8.4.2 >
On Mon, Jan 13, 2014 at 12:58:54PM +0100, David Herrmann wrote: > Does that -1 ever make sense? We don't support mode-object-hotplugging > so all "drm_crtc" objects are known at initialization time. I'd rather > put a BUG() here than a silent -1. This also makes drm_crtc_mask() > redundant. I disagree with that last statement. drm_crtc_mask() is still useful for converting to the mask, rather than having that open coded all over the place. It probably makes more sense for drm_crtc_mask() to become a helper in a header file though.
On Mon, 13 Jan 2014, Thierry Reding <thierry.reding@gmail.com> wrote: > From: Russell King <rmk+kernel@arm.linux.org.uk> > > The encoder possible_crtcs mask identifies which CRTCs can be bound to > a particular encoder. Each bit from bit 0 defines an index in the list > of CRTCs held in the DRM mode_config crtc_list. Rather than having > drivers trying to track the position of their CRTCs in the list, expose > the code which already exists for calculating the appropriate mask bit > for a CRTC. > > Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk> > Reviewed-by: David Herrmann <dh.herrmann@gmail.com> > [treding@nvidia.com: add drm_crtc_index(), move to core] > Signed-off-by: Thierry Reding <treding@nvidia.com> > --- > Changes in v2: > - further extract a new drm_crtc_index() function > - move to DRM core (drm_crtc.c) > > drivers/gpu/drm/drm_crtc.c | 38 ++++++++++++++++++++++++++++++++++++++ > drivers/gpu/drm/drm_crtc_helper.c | 16 +++------------- > include/drm/drm_crtc.h | 2 ++ > 3 files changed, 43 insertions(+), 13 deletions(-) > > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c > index 266a01d7f635..fdfa5a0e51d6 100644 > --- a/drivers/gpu/drm/drm_crtc.c > +++ b/drivers/gpu/drm/drm_crtc.c > @@ -675,6 +675,44 @@ void drm_crtc_cleanup(struct drm_crtc *crtc) > EXPORT_SYMBOL(drm_crtc_cleanup); > > /** > + * drm_crtc_index - find the index of a registered CRTC > + * @crtc: CRTC to find index for > + * > + * Given a registered CRTC, return the index of that CRTC within a DRM > + * device's list of CRTCs. If the CRTC isn't registered, return -1. > + */ > +int drm_crtc_index(struct drm_crtc *crtc) > +{ > + struct drm_crtc *tmp; > + int index = 0; > + > + list_for_each_entry(tmp, &crtc->dev->mode_config.crtc_list, head) { > + if (tmp == crtc) > + return index; > + > + index++; > + } > + > + return -1; I share David's opinion of using BUG() here. > +} > +EXPORT_SYMBOL(drm_crtc_index); > + > +/** > + * drm_crtc_mask - find the mask of a registered CRTC > + * @crtc: CRTC to find mask for > + * > + * Given a registered CRTC, return the mask bit of that CRTC for an > + * encoder's possible_crtcs field. > + */ > +uint32_t drm_crtc_mask(struct drm_crtc *crtc) > +{ > + int i = drm_crtc_index(crtc); > + > + return i < 0 ? 0 : 1 << i; > +} > +EXPORT_SYMBOL(drm_crtc_mask); > + > +/** > * drm_mode_probed_add - add a mode to a connector's probed mode list > * @connector: connector the new mode > * @mode: mode data > diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c > index 01361aba033b..95b32098badf 100644 > --- a/drivers/gpu/drm/drm_crtc_helper.c > +++ b/drivers/gpu/drm/drm_crtc_helper.c > @@ -334,23 +334,13 @@ EXPORT_SYMBOL(drm_helper_disable_unused_functions); > static bool drm_encoder_crtc_ok(struct drm_encoder *encoder, > struct drm_crtc *crtc) > { > - struct drm_device *dev; > - struct drm_crtc *tmp; > - int crtc_mask = 1; > + uint32_t crtc_mask; > > WARN(!crtc, "checking null crtc?\n"); You could drop this while at it, the code will neatly oops a moment later anyway if crtc == NULL. Otherwise, both patches are Reviewed-by: Jani Nikula <jani.nikula@intel.com> > > - dev = crtc->dev; > - > - list_for_each_entry(tmp, &dev->mode_config.crtc_list, head) { > - if (tmp == crtc) > - break; > - crtc_mask <<= 1; > - } > + crtc_mask = drm_crtc_mask(crtc); > > - if (encoder->possible_crtcs & crtc_mask) > - return true; > - return false; > + return !!(encoder->possible_crtcs & crtc_mask); > } > > /* > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h > index f32c5cd51f41..a61fb73fdcb5 100644 > --- a/include/drm/drm_crtc.h > +++ b/include/drm/drm_crtc.h > @@ -929,6 +929,8 @@ extern int drm_crtc_init(struct drm_device *dev, > struct drm_crtc *crtc, > const struct drm_crtc_funcs *funcs); > extern void drm_crtc_cleanup(struct drm_crtc *crtc); > +extern int drm_crtc_index(struct drm_crtc *crtc); > +extern uint32_t drm_crtc_mask(struct drm_crtc *crtc); > > extern void drm_connector_ida_init(void); > extern void drm_connector_ida_destroy(void); > -- > 1.8.4.2 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Hi Russel On Mon, Jan 13, 2014 at 1:47 PM, Russell King - ARM Linux <linux@arm.linux.org.uk> wrote: > On Mon, Jan 13, 2014 at 12:58:54PM +0100, David Herrmann wrote: >> Does that -1 ever make sense? We don't support mode-object-hotplugging >> so all "drm_crtc" objects are known at initialization time. I'd rather >> put a BUG() here than a silent -1. This also makes drm_crtc_mask() >> redundant. > > I disagree with that last statement. drm_crtc_mask() is still useful > for converting to the mask, rather than having that open coded all > over the place. It probably makes more sense for drm_crtc_mask() to > become a helper in a header file though. Thierry renamed your helper to drm_crtc_index() and added drm_crtc_mask(). If we remove the -1, drm_crtc_mask() is redundant as it just calls drm_crtc_index(). So I cannot follow what you mean by "open coded all over the place". I guess you were talking about drm_crtc_index()? Thanks David
On Mon, Jan 13, 2014 at 02:50:46PM +0100, David Herrmann wrote: > Hi Russel > > On Mon, Jan 13, 2014 at 1:47 PM, Russell King - ARM Linux > <linux@arm.linux.org.uk> wrote: > > On Mon, Jan 13, 2014 at 12:58:54PM +0100, David Herrmann wrote: > >> Does that -1 ever make sense? We don't support mode-object-hotplugging > >> so all "drm_crtc" objects are known at initialization time. I'd rather > >> put a BUG() here than a silent -1. This also makes drm_crtc_mask() > >> redundant. > > > > I disagree with that last statement. drm_crtc_mask() is still useful > > for converting to the mask, rather than having that open coded all > > over the place. It probably makes more sense for drm_crtc_mask() to > > become a helper in a header file though. > > Thierry renamed your helper to drm_crtc_index() and added > drm_crtc_mask(). If we remove the -1, drm_crtc_mask() is redundant as > it just calls drm_crtc_index(). So I cannot follow what you mean by > "open coded all over the place". I guess you were talking about > drm_crtc_index()? +uint32_t drm_crtc_mask(struct drm_crtc *crtc) +{ + int i = drm_crtc_index(crtc); + + return i < 0 ? 0 : 1 << i; +} +EXPORT_SYMBOL(drm_crtc_mask); When drm_crtc_index() no longer returns negative numbers, this becomes: +uint32_t drm_crtc_mask(struct drm_crtc *crtc) +{ + return 1 << drm_crtc_index(crtc); +} +EXPORT_SYMBOL(drm_crtc_mask); Notice the conversion from an index returned by drm_crtc_index() to a bitmask.
Hi On Mon, Jan 13, 2014 at 2:55 PM, Russell King - ARM Linux <linux@arm.linux.org.uk> wrote: > On Mon, Jan 13, 2014 at 02:50:46PM +0100, David Herrmann wrote: >> Hi Russel >> >> On Mon, Jan 13, 2014 at 1:47 PM, Russell King - ARM Linux >> <linux@arm.linux.org.uk> wrote: >> > On Mon, Jan 13, 2014 at 12:58:54PM +0100, David Herrmann wrote: >> >> Does that -1 ever make sense? We don't support mode-object-hotplugging >> >> so all "drm_crtc" objects are known at initialization time. I'd rather >> >> put a BUG() here than a silent -1. This also makes drm_crtc_mask() >> >> redundant. >> > >> > I disagree with that last statement. drm_crtc_mask() is still useful >> > for converting to the mask, rather than having that open coded all >> > over the place. It probably makes more sense for drm_crtc_mask() to >> > become a helper in a header file though. >> >> Thierry renamed your helper to drm_crtc_index() and added >> drm_crtc_mask(). If we remove the -1, drm_crtc_mask() is redundant as >> it just calls drm_crtc_index(). So I cannot follow what you mean by >> "open coded all over the place". I guess you were talking about >> drm_crtc_index()? > > +uint32_t drm_crtc_mask(struct drm_crtc *crtc) > +{ > + int i = drm_crtc_index(crtc); > + > + return i < 0 ? 0 : 1 << i; > +} > +EXPORT_SYMBOL(drm_crtc_mask); > > When drm_crtc_index() no longer returns negative numbers, this becomes: > > +uint32_t drm_crtc_mask(struct drm_crtc *crtc) > +{ > + return 1 << drm_crtc_index(crtc); > +} > +EXPORT_SYMBOL(drm_crtc_mask); > > Notice the conversion from an index returned by drm_crtc_index() to a > bitmask. Oops, missed that, sorry for the noise.
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 266a01d7f635..fdfa5a0e51d6 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -675,6 +675,44 @@ void drm_crtc_cleanup(struct drm_crtc *crtc) EXPORT_SYMBOL(drm_crtc_cleanup); /** + * drm_crtc_index - find the index of a registered CRTC + * @crtc: CRTC to find index for + * + * Given a registered CRTC, return the index of that CRTC within a DRM + * device's list of CRTCs. If the CRTC isn't registered, return -1. + */ +int drm_crtc_index(struct drm_crtc *crtc) +{ + struct drm_crtc *tmp; + int index = 0; + + list_for_each_entry(tmp, &crtc->dev->mode_config.crtc_list, head) { + if (tmp == crtc) + return index; + + index++; + } + + return -1; +} +EXPORT_SYMBOL(drm_crtc_index); + +/** + * drm_crtc_mask - find the mask of a registered CRTC + * @crtc: CRTC to find mask for + * + * Given a registered CRTC, return the mask bit of that CRTC for an + * encoder's possible_crtcs field. + */ +uint32_t drm_crtc_mask(struct drm_crtc *crtc) +{ + int i = drm_crtc_index(crtc); + + return i < 0 ? 0 : 1 << i; +} +EXPORT_SYMBOL(drm_crtc_mask); + +/** * drm_mode_probed_add - add a mode to a connector's probed mode list * @connector: connector the new mode * @mode: mode data diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c index 01361aba033b..95b32098badf 100644 --- a/drivers/gpu/drm/drm_crtc_helper.c +++ b/drivers/gpu/drm/drm_crtc_helper.c @@ -334,23 +334,13 @@ EXPORT_SYMBOL(drm_helper_disable_unused_functions); static bool drm_encoder_crtc_ok(struct drm_encoder *encoder, struct drm_crtc *crtc) { - struct drm_device *dev; - struct drm_crtc *tmp; - int crtc_mask = 1; + uint32_t crtc_mask; WARN(!crtc, "checking null crtc?\n"); - dev = crtc->dev; - - list_for_each_entry(tmp, &dev->mode_config.crtc_list, head) { - if (tmp == crtc) - break; - crtc_mask <<= 1; - } + crtc_mask = drm_crtc_mask(crtc); - if (encoder->possible_crtcs & crtc_mask) - return true; - return false; + return !!(encoder->possible_crtcs & crtc_mask); } /* diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index f32c5cd51f41..a61fb73fdcb5 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -929,6 +929,8 @@ extern int drm_crtc_init(struct drm_device *dev, struct drm_crtc *crtc, const struct drm_crtc_funcs *funcs); extern void drm_crtc_cleanup(struct drm_crtc *crtc); +extern int drm_crtc_index(struct drm_crtc *crtc); +extern uint32_t drm_crtc_mask(struct drm_crtc *crtc); extern void drm_connector_ida_init(void); extern void drm_connector_ida_destroy(void);