| Message ID | E1Vypnh-0007Ev-3v@rmk-PC.arm.linux.org.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi On Thu, Jan 2, 2014 at 10:27 PM, Russell King <rmk+kernel@arm.linux.org.uk> wrote: > 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> > --- > drivers/gpu/drm/drm_crtc_helper.c | 39 ++++++++++++++++++++++++------------ > include/drm/drm_crtc_helper.h | 1 + > 2 files changed, 27 insertions(+), 13 deletions(-) > > diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c > index 01361aba033b..10708c248196 100644 > --- a/drivers/gpu/drm/drm_crtc_helper.c > +++ b/drivers/gpu/drm/drm_crtc_helper.c > @@ -325,6 +325,29 @@ void drm_helper_disable_unused_functions(struct drm_device *dev) > EXPORT_SYMBOL(drm_helper_disable_unused_functions); > > /** > + * drm_helper_crtc_possible_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. I think this is a nice cleanup which we could pick up right away. Most drivers can be simplified by using this. Only the name is misleading, imo, I'd use something like drm_helper_crtc_to_mask(). Anyhow, not my call so: Reviewed-by: David Herrmann <dh.herrmann@gmail.com> Thanks David > + */ > +uint32_t drm_helper_crtc_possible_mask(struct drm_crtc *crtc) > +{ > + struct drm_device *dev = crtc->dev; > + struct drm_crtc *tmp; > + uint32_t crtc_mask = 1; > + > + list_for_each_entry(tmp, &dev->mode_config.crtc_list, head) { > + if (tmp == crtc) > + return crtc_mask; > + crtc_mask <<= 1; > + } > + > + return 0; > +} > +EXPORT_SYMBOL(drm_helper_crtc_possible_mask); > + > +/** > * drm_encoder_crtc_ok - can a given crtc drive a given encoder? > * @encoder: encoder to test > * @crtc: crtc to test > @@ -334,23 +357,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_helper_crtc_possible_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_helper.h b/include/drm/drm_crtc_helper.h > index ef6ad3a8e58e..21b9f47dd88c 100644 > --- a/include/drm/drm_crtc_helper.h > +++ b/include/drm/drm_crtc_helper.h > @@ -127,6 +127,7 @@ struct drm_connector_helper_funcs { > > extern int drm_helper_probe_single_connector_modes(struct drm_connector *connector, uint32_t maxX, uint32_t maxY); > extern void drm_helper_disable_unused_functions(struct drm_device *dev); > +extern uint32_t drm_helper_crtc_possible_mask(struct drm_crtc *crtc); > extern int drm_crtc_helper_set_config(struct drm_mode_set *set); > extern bool drm_crtc_helper_set_mode(struct drm_crtc *crtc, > struct drm_display_mode *mode, > -- > 1.7.4.4 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel
On Fri, Jan 03, 2014 at 05:05:46PM +0100, David Herrmann wrote: > Hi > > On Thu, Jan 2, 2014 at 10:27 PM, Russell King > <rmk+kernel@arm.linux.org.uk> wrote: > > 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> > > --- > > drivers/gpu/drm/drm_crtc_helper.c | 39 ++++++++++++++++++++++++------------ > > include/drm/drm_crtc_helper.h | 1 + > > 2 files changed, 27 insertions(+), 13 deletions(-) > > > > diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c > > index 01361aba033b..10708c248196 100644 > > --- a/drivers/gpu/drm/drm_crtc_helper.c > > +++ b/drivers/gpu/drm/drm_crtc_helper.c > > @@ -325,6 +325,29 @@ void drm_helper_disable_unused_functions(struct drm_device *dev) > > EXPORT_SYMBOL(drm_helper_disable_unused_functions); > > > > /** > > + * drm_helper_crtc_possible_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. > > I think this is a nice cleanup which we could pick up right away. Most > drivers can be simplified by using this. Only the name is misleading, > imo, I'd use something like drm_helper_crtc_to_mask(). I'm not so sure - since the only place this mask gets used is with the "possible_crtcs" field. It's got nothing to do with the "possible_clones" field as that isn't a mask of CRTCs.
Hi On Fri, Jan 3, 2014 at 5:13 PM, Russell King - ARM Linux <linux@arm.linux.org.uk> wrote: > On Fri, Jan 03, 2014 at 05:05:46PM +0100, David Herrmann wrote: >> Hi >> >> On Thu, Jan 2, 2014 at 10:27 PM, Russell King >> <rmk+kernel@arm.linux.org.uk> wrote: >> > 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> >> > --- >> > drivers/gpu/drm/drm_crtc_helper.c | 39 ++++++++++++++++++++++++------------ >> > include/drm/drm_crtc_helper.h | 1 + >> > 2 files changed, 27 insertions(+), 13 deletions(-) >> > >> > diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c >> > index 01361aba033b..10708c248196 100644 >> > --- a/drivers/gpu/drm/drm_crtc_helper.c >> > +++ b/drivers/gpu/drm/drm_crtc_helper.c >> > @@ -325,6 +325,29 @@ void drm_helper_disable_unused_functions(struct drm_device *dev) >> > EXPORT_SYMBOL(drm_helper_disable_unused_functions); >> > >> > /** >> > + * drm_helper_crtc_possible_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. >> >> I think this is a nice cleanup which we could pick up right away. Most >> drivers can be simplified by using this. Only the name is misleading, >> imo, I'd use something like drm_helper_crtc_to_mask(). > > I'm not so sure - since the only place this mask gets used is with > the "possible_crtcs" field. It's got nothing to do with the > "possible_clones" field as that isn't a mask of CRTCs. At least intel's copy of intel_crtc_encoder_ok() can be updated, too. But they don't depend on the crtc_helpers so we'd have to move your small helper into drm_crtc.c (which is fine to me as it is not limited to the DRM-crtc-helpers). possible_clones is about encoders, you're right, I missed that. So you can carry it in your series just fine. Thanks David
On Fri, Jan 03, 2014 at 05:26:14PM +0100, David Herrmann wrote: > Hi > > On Fri, Jan 3, 2014 at 5:13 PM, Russell King - ARM Linux > <linux@arm.linux.org.uk> wrote: > > I'm not so sure - since the only place this mask gets used is with > > the "possible_crtcs" field. It's got nothing to do with the > > "possible_clones" field as that isn't a mask of CRTCs. > ... > possible_clones is about encoders, you're right, I missed that. So you > can carry it in your series just fine. Thanks for confirming that - it's something which imx-drm seemed to get wrong as put the same value into possible_clones as it did for possible_crtcs. I was fairly certain that was buggy. :)
diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c index 01361aba033b..10708c248196 100644 --- a/drivers/gpu/drm/drm_crtc_helper.c +++ b/drivers/gpu/drm/drm_crtc_helper.c @@ -325,6 +325,29 @@ void drm_helper_disable_unused_functions(struct drm_device *dev) EXPORT_SYMBOL(drm_helper_disable_unused_functions); /** + * drm_helper_crtc_possible_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_helper_crtc_possible_mask(struct drm_crtc *crtc) +{ + struct drm_device *dev = crtc->dev; + struct drm_crtc *tmp; + uint32_t crtc_mask = 1; + + list_for_each_entry(tmp, &dev->mode_config.crtc_list, head) { + if (tmp == crtc) + return crtc_mask; + crtc_mask <<= 1; + } + + return 0; +} +EXPORT_SYMBOL(drm_helper_crtc_possible_mask); + +/** * drm_encoder_crtc_ok - can a given crtc drive a given encoder? * @encoder: encoder to test * @crtc: crtc to test @@ -334,23 +357,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_helper_crtc_possible_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_helper.h b/include/drm/drm_crtc_helper.h index ef6ad3a8e58e..21b9f47dd88c 100644 --- a/include/drm/drm_crtc_helper.h +++ b/include/drm/drm_crtc_helper.h @@ -127,6 +127,7 @@ struct drm_connector_helper_funcs { extern int drm_helper_probe_single_connector_modes(struct drm_connector *connector, uint32_t maxX, uint32_t maxY); extern void drm_helper_disable_unused_functions(struct drm_device *dev); +extern uint32_t drm_helper_crtc_possible_mask(struct drm_crtc *crtc); extern int drm_crtc_helper_set_config(struct drm_mode_set *set); extern bool drm_crtc_helper_set_mode(struct drm_crtc *crtc, struct drm_display_mode *mode,
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> --- drivers/gpu/drm/drm_crtc_helper.c | 39 ++++++++++++++++++++++++------------ include/drm/drm_crtc_helper.h | 1 + 2 files changed, 27 insertions(+), 13 deletions(-)