public inbox for intel-gfx@lists.freedesktop.org
 help / color / mirror / Atom feed
* [PATCH 0/2] drm: Introduce drm_crtc_index() and drm_crtc_mask()
@ 2014-01-13 11:46 Thierry Reding
  2014-01-13 11:46 ` [PATCH 1/2] drm: provide a helper for the encoder possible_crtcs mask Thierry Reding
  2014-01-13 11:46 ` [PATCH 2/2] drm/i915: Use drm_crtc_mask() helper Thierry Reding
  0 siblings, 2 replies; 9+ messages in thread
From: Thierry Reding @ 2014-01-13 11:46 UTC (permalink / raw)
  To: dri-devel; +Cc: Daniel Vetter, intel-gfx, Russell King

This series is a continuation of a patch posted earlier by Russell. It
addresses some comments from the initial posting as well as from some
discussion on IRC.

Patch 2 makes use of the new functions to replace an open-coded version
in the i915 driver, as suggested by David Herrmann. I've left David's
Reviewed-by in place on patch 1 since it didn't change fundamentally.

Note that I have a pair of patches that rely on this to fix a bug in the
Tegra driver that was exposed by the addition of the panel support, so I
would like to take these two patches through the Tegra tree.

Thierry

Russell King (1):
  drm: provide a helper for the encoder possible_crtcs mask

Thierry Reding (1):
  drm/i915: Use drm_crtc_mask() helper

 drivers/gpu/drm/drm_crtc.c           | 38 ++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/drm_crtc_helper.c    | 16 +++------------
 drivers/gpu/drm/i915/intel_display.c | 16 +--------------
 include/drm/drm_crtc.h               |  2 ++
 4 files changed, 44 insertions(+), 28 deletions(-)

-- 
1.8.4.2

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH 1/2] drm: provide a helper for the encoder possible_crtcs mask
  2014-01-13 11:46 [PATCH 0/2] drm: Introduce drm_crtc_index() and drm_crtc_mask() Thierry Reding
@ 2014-01-13 11:46 ` Thierry Reding
  2014-01-13 11:58   ` David Herrmann
  2014-01-13 13:14   ` Jani Nikula
  2014-01-13 11:46 ` [PATCH 2/2] drm/i915: Use drm_crtc_mask() helper Thierry Reding
  1 sibling, 2 replies; 9+ messages in thread
From: Thierry Reding @ 2014-01-13 11:46 UTC (permalink / raw)
  To: dri-devel
  Cc: David Airlie, Daniel Vetter, intel-gfx, David Herrmann,
	Russell King

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;
+}
+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

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH 2/2] drm/i915: Use drm_crtc_mask() helper
  2014-01-13 11:46 [PATCH 0/2] drm: Introduce drm_crtc_index() and drm_crtc_mask() Thierry Reding
  2014-01-13 11:46 ` [PATCH 1/2] drm: provide a helper for the encoder possible_crtcs mask Thierry Reding
@ 2014-01-13 11:46 ` Thierry Reding
  1 sibling, 0 replies; 9+ messages in thread
From: Thierry Reding @ 2014-01-13 11:46 UTC (permalink / raw)
  To: dri-devel
  Cc: David Airlie, Daniel Vetter, intel-gfx, David Herrmann,
	Russell King

Replace an open-coded occurrence by a call to the new drm_crtc_mask()
helper.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/gpu/drm/i915/intel_display.c | 16 +---------------
 1 file changed, 1 insertion(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index e77d4b8856a7..807feab38ea6 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -8723,21 +8723,7 @@ static struct drm_crtc_helper_funcs intel_helper_funcs = {
 static bool intel_encoder_crtc_ok(struct drm_encoder *encoder,
 				  struct drm_crtc *crtc)
 {
-	struct drm_device *dev;
-	struct drm_crtc *tmp;
-	int crtc_mask = 1;
-
-	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;
-	}
-
-	if (encoder->possible_crtcs & crtc_mask)
+	if (encoder->possible_crtcs & drm_crtc_mask(crtc))
 		return true;
 	return false;
 }
-- 
1.8.4.2

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/2] drm: provide a helper for the encoder possible_crtcs mask
  2014-01-13 11:46 ` [PATCH 1/2] drm: provide a helper for the encoder possible_crtcs mask Thierry Reding
@ 2014-01-13 11:58   ` David Herrmann
  2014-01-13 12:47     ` Russell King - ARM Linux
  2014-01-13 13:14   ` Jani Nikula
  1 sibling, 1 reply; 9+ messages in thread
From: David Herrmann @ 2014-01-13 11:58 UTC (permalink / raw)
  To: Thierry Reding
  Cc: Daniel Vetter, Intel Graphics Development,
	dri-devel@lists.freedesktop.org, Russell King

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
>

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/2] drm: provide a helper for the encoder possible_crtcs mask
  2014-01-13 11:58   ` David Herrmann
@ 2014-01-13 12:47     ` Russell King - ARM Linux
  2014-01-13 13:50       ` David Herrmann
  0 siblings, 1 reply; 9+ messages in thread
From: Russell King - ARM Linux @ 2014-01-13 12:47 UTC (permalink / raw)
  To: David Herrmann
  Cc: Daniel Vetter, Intel Graphics Development,
	dri-devel@lists.freedesktop.org

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.

-- 
FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up.  Estimation
in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad.
Estimate before purchase was "up to 13.2Mbit".

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/2] drm: provide a helper for the encoder possible_crtcs mask
  2014-01-13 11:46 ` [PATCH 1/2] drm: provide a helper for the encoder possible_crtcs mask Thierry Reding
  2014-01-13 11:58   ` David Herrmann
@ 2014-01-13 13:14   ` Jani Nikula
  1 sibling, 0 replies; 9+ messages in thread
From: Jani Nikula @ 2014-01-13 13:14 UTC (permalink / raw)
  To: Thierry Reding, dri-devel
  Cc: David Airlie, Daniel Vetter, intel-gfx, Russell King,
	David Herrmann

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

-- 
Jani Nikula, Intel Open Source Technology Center

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/2] drm: provide a helper for the encoder possible_crtcs mask
  2014-01-13 12:47     ` Russell King - ARM Linux
@ 2014-01-13 13:50       ` David Herrmann
  2014-01-13 13:55         ` Russell King - ARM Linux
  0 siblings, 1 reply; 9+ messages in thread
From: David Herrmann @ 2014-01-13 13:50 UTC (permalink / raw)
  To: Russell King - ARM Linux
  Cc: David Airlie, Daniel Vetter, Intel Graphics Development,
	dri-devel@lists.freedesktop.org, Thierry Reding

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/2] drm: provide a helper for the encoder possible_crtcs mask
  2014-01-13 13:50       ` David Herrmann
@ 2014-01-13 13:55         ` Russell King - ARM Linux
  2014-01-13 13:57           ` David Herrmann
  0 siblings, 1 reply; 9+ messages in thread
From: Russell King - ARM Linux @ 2014-01-13 13:55 UTC (permalink / raw)
  To: David Herrmann
  Cc: Daniel Vetter, Intel Graphics Development,
	dri-devel@lists.freedesktop.org

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.

-- 
FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up.  Estimation
in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad.
Estimate before purchase was "up to 13.2Mbit".

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/2] drm: provide a helper for the encoder possible_crtcs mask
  2014-01-13 13:55         ` Russell King - ARM Linux
@ 2014-01-13 13:57           ` David Herrmann
  0 siblings, 0 replies; 9+ messages in thread
From: David Herrmann @ 2014-01-13 13:57 UTC (permalink / raw)
  To: Russell King - ARM Linux
  Cc: David Airlie, Daniel Vetter, Intel Graphics Development,
	dri-devel@lists.freedesktop.org, Thierry Reding

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.

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2014-01-13 13:57 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-01-13 11:46 [PATCH 0/2] drm: Introduce drm_crtc_index() and drm_crtc_mask() Thierry Reding
2014-01-13 11:46 ` [PATCH 1/2] drm: provide a helper for the encoder possible_crtcs mask Thierry Reding
2014-01-13 11:58   ` David Herrmann
2014-01-13 12:47     ` Russell King - ARM Linux
2014-01-13 13:50       ` David Herrmann
2014-01-13 13:55         ` Russell King - ARM Linux
2014-01-13 13:57           ` David Herrmann
2014-01-13 13:14   ` Jani Nikula
2014-01-13 11:46 ` [PATCH 2/2] drm/i915: Use drm_crtc_mask() helper Thierry Reding

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox