* [PATCH 1/2] drm/i915: Copy PCI device id into the device info block
@ 2014-08-09 18:18 Chris Wilson
2014-08-09 18:18 ` [PATCH 2/2] drm/i915: Agnostic INTEL_INFO Chris Wilson
2014-08-11 10:05 ` [PATCH 1/2] drm/i915: Copy PCI device id into the device info block Jani Nikula
0 siblings, 2 replies; 8+ messages in thread
From: Chris Wilson @ 2014-08-09 18:18 UTC (permalink / raw)
To: intel-gfx
This is so that we can make the drm_i915_private->info always the
preferred source for chipset type and feature queries.
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
drivers/gpu/drm/i915/i915_dma.c | 5 +++--
drivers/gpu/drm/i915/i915_drv.h | 50 +++++++++++++++++++++--------------------
2 files changed, 29 insertions(+), 26 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 96bf11e7a011..39988940d468 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1592,9 +1592,10 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
dev->dev_private = dev_priv;
dev_priv->dev = dev;
- /* copy initial configuration to dev_priv->info */
+ /* Setup the write-once "constant" device info */
device_info = (struct intel_device_info *)&dev_priv->info;
- *device_info = *info;
+ memcpy(device_info, info, sizeof(dev_priv->info));
+ device_info->device_id = dev->pdev->device;
spin_lock_init(&dev_priv->irq_lock);
spin_lock_init(&dev_priv->gpu_error.lock);
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index b3d42b085790..0aef763ffa75 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -563,6 +563,7 @@ struct intel_uncore {
struct intel_device_info {
u32 display_mmio_offset;
+ u16 device_id;
u8 num_pipes:3;
u8 num_sprites[I915_MAX_PIPES];
u8 gen;
@@ -2062,51 +2063,52 @@ struct drm_i915_cmd_table {
int count;
};
-#define INTEL_INFO(dev) (&to_i915(dev)->info)
+#define INTEL_INFO(p) (&to_i915(p)->info)
+#define INTEL_DEVID(p) (INTEL_INFO(p)->device_id)
-#define IS_I830(dev) ((dev)->pdev->device == 0x3577)
-#define IS_845G(dev) ((dev)->pdev->device == 0x2562)
+#define IS_I830(dev) (INTEL_DEVID(dev) == 0x3577)
+#define IS_845G(dev) (INTEL_DEVID(dev) == 0x2562)
#define IS_I85X(dev) (INTEL_INFO(dev)->is_i85x)
-#define IS_I865G(dev) ((dev)->pdev->device == 0x2572)
+#define IS_I865G(dev) (INTEL_DEVID(dev) == 0x2572)
#define IS_I915G(dev) (INTEL_INFO(dev)->is_i915g)
-#define IS_I915GM(dev) ((dev)->pdev->device == 0x2592)
-#define IS_I945G(dev) ((dev)->pdev->device == 0x2772)
+#define IS_I915GM(dev) (INTEL_DEVID(dev) == 0x2592)
+#define IS_I945G(dev) (INTEL_DEVID(dev) == 0x2772)
#define IS_I945GM(dev) (INTEL_INFO(dev)->is_i945gm)
#define IS_BROADWATER(dev) (INTEL_INFO(dev)->is_broadwater)
#define IS_CRESTLINE(dev) (INTEL_INFO(dev)->is_crestline)
-#define IS_GM45(dev) ((dev)->pdev->device == 0x2A42)
+#define IS_GM45(dev) (INTEL_DEVID(dev) == 0x2A42)
#define IS_G4X(dev) (INTEL_INFO(dev)->is_g4x)
-#define IS_PINEVIEW_G(dev) ((dev)->pdev->device == 0xa001)
-#define IS_PINEVIEW_M(dev) ((dev)->pdev->device == 0xa011)
+#define IS_PINEVIEW_G(dev) (INTEL_DEVID(dev) == 0xa001)
+#define IS_PINEVIEW_M(dev) (INTEL_DEVID(dev) == 0xa011)
#define IS_PINEVIEW(dev) (INTEL_INFO(dev)->is_pineview)
#define IS_G33(dev) (INTEL_INFO(dev)->is_g33)
-#define IS_IRONLAKE_M(dev) ((dev)->pdev->device == 0x0046)
+#define IS_IRONLAKE_M(dev) (INTEL_DEVID(dev) == 0x0046)
#define IS_IVYBRIDGE(dev) (INTEL_INFO(dev)->is_ivybridge)
-#define IS_IVB_GT1(dev) ((dev)->pdev->device == 0x0156 || \
- (dev)->pdev->device == 0x0152 || \
- (dev)->pdev->device == 0x015a)
-#define IS_SNB_GT1(dev) ((dev)->pdev->device == 0x0102 || \
- (dev)->pdev->device == 0x0106 || \
- (dev)->pdev->device == 0x010A)
+#define IS_IVB_GT1(dev) (INTEL_DEVID(dev) == 0x0156 || \
+ INTEL_DEVID(dev) == 0x0152 || \
+ INTEL_DEVID(dev) == 0x015a)
+#define IS_SNB_GT1(dev) (INTEL_DEVID(dev) == 0x0102 || \
+ INTEL_DEVID(dev) == 0x0106 || \
+ INTEL_DEVID(dev) == 0x010A)
#define IS_VALLEYVIEW(dev) (INTEL_INFO(dev)->is_valleyview)
#define IS_CHERRYVIEW(dev) (INTEL_INFO(dev)->is_valleyview && IS_GEN8(dev))
#define IS_HASWELL(dev) (INTEL_INFO(dev)->is_haswell)
#define IS_BROADWELL(dev) (!INTEL_INFO(dev)->is_valleyview && IS_GEN8(dev))
#define IS_MOBILE(dev) (INTEL_INFO(dev)->is_mobile)
#define IS_HSW_EARLY_SDV(dev) (IS_HASWELL(dev) && \
- ((dev)->pdev->device & 0xFF00) == 0x0C00)
+ (INTEL_DEVID(dev) & 0xFF00) == 0x0C00)
#define IS_BDW_ULT(dev) (IS_BROADWELL(dev) && \
- (((dev)->pdev->device & 0xf) == 0x2 || \
- ((dev)->pdev->device & 0xf) == 0x6 || \
- ((dev)->pdev->device & 0xf) == 0xe))
+ ((INTEL_DEVID(dev) & 0xf) == 0x2 || \
+ (INTEL_DEVID(dev) & 0xf) == 0x6 || \
+ (INTEL_DEVID(dev) & 0xf) == 0xe))
#define IS_HSW_ULT(dev) (IS_HASWELL(dev) && \
- ((dev)->pdev->device & 0xFF00) == 0x0A00)
+ (INTEL_DEVID(dev) & 0xFF00) == 0x0A00)
#define IS_ULT(dev) (IS_HSW_ULT(dev) || IS_BDW_ULT(dev))
#define IS_HSW_GT3(dev) (IS_HASWELL(dev) && \
- ((dev)->pdev->device & 0x00F0) == 0x0020)
+ (INTEL_DEVID(dev) & 0x00F0) == 0x0020)
/* ULX machines are also considered ULT. */
-#define IS_HSW_ULX(dev) ((dev)->pdev->device == 0x0A0E || \
- (dev)->pdev->device == 0x0A1E)
+#define IS_HSW_ULX(dev) (INTEL_DEVID(dev) == 0x0A0E || \
+ INTEL_DEVID(dev) == 0x0A1E)
#define IS_PRELIMINARY_HW(intel_info) ((intel_info)->is_preliminary)
/*
--
2.1.0.rc1
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 2/2] drm/i915: Agnostic INTEL_INFO
2014-08-09 18:18 [PATCH 1/2] drm/i915: Copy PCI device id into the device info block Chris Wilson
@ 2014-08-09 18:18 ` Chris Wilson
2014-08-11 10:14 ` Jani Nikula
2014-08-11 10:05 ` [PATCH 1/2] drm/i915: Copy PCI device id into the device info block Jani Nikula
1 sibling, 1 reply; 8+ messages in thread
From: Chris Wilson @ 2014-08-09 18:18 UTC (permalink / raw)
To: intel-gfx
Adapt the macro so that we can pass either the struct drm_device or the
struct drm_i915_private pointers and get the answer we want. Over time,
my plan is to convert all users over to using drm_i915_private and so
trimming down the pointer dance. Having spent a few hours chasing that
goal and achieved over 8k of object code saving, it appears to be a
worthwhile target. This interim macro allows us to slowly convert over.
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
drivers/gpu/drm/i915/i915_dma.c | 3 +++
drivers/gpu/drm/i915/i915_drv.h | 3 ++-
2 files changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 39988940d468..49149406fcd8 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1585,6 +1585,9 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
if (!drm_core_check_feature(dev, DRIVER_MODESET) && !dev->agp)
return -EINVAL;
+ /* For the ugly agnostic INTEL_INFO macro */
+ BUILD_BUG_ON(sizeof(*dev_priv) == sizeof(*dev));
+
dev_priv = kzalloc(sizeof(*dev_priv), GFP_KERNEL);
if (dev_priv == NULL)
return -ENOMEM;
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 0aef763ffa75..e66465430bdc 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2063,7 +2063,8 @@ struct drm_i915_cmd_table {
int count;
};
-#define INTEL_INFO(p) (&to_i915(p)->info)
+#define __I915__(p) ((sizeof(*(p)) == sizeof(struct drm_i915_private)) ? (struct drm_i915_private *)(p) : to_i915((struct drm_device *)p))
+#define INTEL_INFO(p) (&__I915__(p)->info)
#define INTEL_DEVID(p) (INTEL_INFO(p)->device_id)
#define IS_I830(dev) (INTEL_DEVID(dev) == 0x3577)
--
2.1.0.rc1
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] drm/i915: Copy PCI device id into the device info block
2014-08-09 18:18 [PATCH 1/2] drm/i915: Copy PCI device id into the device info block Chris Wilson
2014-08-09 18:18 ` [PATCH 2/2] drm/i915: Agnostic INTEL_INFO Chris Wilson
@ 2014-08-11 10:05 ` Jani Nikula
2014-08-11 11:26 ` Daniel Vetter
1 sibling, 1 reply; 8+ messages in thread
From: Jani Nikula @ 2014-08-11 10:05 UTC (permalink / raw)
To: Chris Wilson, intel-gfx
On Sat, 09 Aug 2014, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> This is so that we can make the drm_i915_private->info always the
> preferred source for chipset type and feature queries.
Reviewed-by: Jani Nikula <jani.nikula@intel.com>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
> drivers/gpu/drm/i915/i915_dma.c | 5 +++--
> drivers/gpu/drm/i915/i915_drv.h | 50 +++++++++++++++++++++--------------------
> 2 files changed, 29 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index 96bf11e7a011..39988940d468 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -1592,9 +1592,10 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
> dev->dev_private = dev_priv;
> dev_priv->dev = dev;
>
> - /* copy initial configuration to dev_priv->info */
> + /* Setup the write-once "constant" device info */
> device_info = (struct intel_device_info *)&dev_priv->info;
> - *device_info = *info;
> + memcpy(device_info, info, sizeof(dev_priv->info));
> + device_info->device_id = dev->pdev->device;
>
> spin_lock_init(&dev_priv->irq_lock);
> spin_lock_init(&dev_priv->gpu_error.lock);
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index b3d42b085790..0aef763ffa75 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -563,6 +563,7 @@ struct intel_uncore {
>
> struct intel_device_info {
> u32 display_mmio_offset;
> + u16 device_id;
> u8 num_pipes:3;
> u8 num_sprites[I915_MAX_PIPES];
> u8 gen;
> @@ -2062,51 +2063,52 @@ struct drm_i915_cmd_table {
> int count;
> };
>
> -#define INTEL_INFO(dev) (&to_i915(dev)->info)
> +#define INTEL_INFO(p) (&to_i915(p)->info)
> +#define INTEL_DEVID(p) (INTEL_INFO(p)->device_id)
>
> -#define IS_I830(dev) ((dev)->pdev->device == 0x3577)
> -#define IS_845G(dev) ((dev)->pdev->device == 0x2562)
> +#define IS_I830(dev) (INTEL_DEVID(dev) == 0x3577)
> +#define IS_845G(dev) (INTEL_DEVID(dev) == 0x2562)
> #define IS_I85X(dev) (INTEL_INFO(dev)->is_i85x)
> -#define IS_I865G(dev) ((dev)->pdev->device == 0x2572)
> +#define IS_I865G(dev) (INTEL_DEVID(dev) == 0x2572)
> #define IS_I915G(dev) (INTEL_INFO(dev)->is_i915g)
> -#define IS_I915GM(dev) ((dev)->pdev->device == 0x2592)
> -#define IS_I945G(dev) ((dev)->pdev->device == 0x2772)
> +#define IS_I915GM(dev) (INTEL_DEVID(dev) == 0x2592)
> +#define IS_I945G(dev) (INTEL_DEVID(dev) == 0x2772)
> #define IS_I945GM(dev) (INTEL_INFO(dev)->is_i945gm)
> #define IS_BROADWATER(dev) (INTEL_INFO(dev)->is_broadwater)
> #define IS_CRESTLINE(dev) (INTEL_INFO(dev)->is_crestline)
> -#define IS_GM45(dev) ((dev)->pdev->device == 0x2A42)
> +#define IS_GM45(dev) (INTEL_DEVID(dev) == 0x2A42)
> #define IS_G4X(dev) (INTEL_INFO(dev)->is_g4x)
> -#define IS_PINEVIEW_G(dev) ((dev)->pdev->device == 0xa001)
> -#define IS_PINEVIEW_M(dev) ((dev)->pdev->device == 0xa011)
> +#define IS_PINEVIEW_G(dev) (INTEL_DEVID(dev) == 0xa001)
> +#define IS_PINEVIEW_M(dev) (INTEL_DEVID(dev) == 0xa011)
> #define IS_PINEVIEW(dev) (INTEL_INFO(dev)->is_pineview)
> #define IS_G33(dev) (INTEL_INFO(dev)->is_g33)
> -#define IS_IRONLAKE_M(dev) ((dev)->pdev->device == 0x0046)
> +#define IS_IRONLAKE_M(dev) (INTEL_DEVID(dev) == 0x0046)
> #define IS_IVYBRIDGE(dev) (INTEL_INFO(dev)->is_ivybridge)
> -#define IS_IVB_GT1(dev) ((dev)->pdev->device == 0x0156 || \
> - (dev)->pdev->device == 0x0152 || \
> - (dev)->pdev->device == 0x015a)
> -#define IS_SNB_GT1(dev) ((dev)->pdev->device == 0x0102 || \
> - (dev)->pdev->device == 0x0106 || \
> - (dev)->pdev->device == 0x010A)
> +#define IS_IVB_GT1(dev) (INTEL_DEVID(dev) == 0x0156 || \
> + INTEL_DEVID(dev) == 0x0152 || \
> + INTEL_DEVID(dev) == 0x015a)
> +#define IS_SNB_GT1(dev) (INTEL_DEVID(dev) == 0x0102 || \
> + INTEL_DEVID(dev) == 0x0106 || \
> + INTEL_DEVID(dev) == 0x010A)
> #define IS_VALLEYVIEW(dev) (INTEL_INFO(dev)->is_valleyview)
> #define IS_CHERRYVIEW(dev) (INTEL_INFO(dev)->is_valleyview && IS_GEN8(dev))
> #define IS_HASWELL(dev) (INTEL_INFO(dev)->is_haswell)
> #define IS_BROADWELL(dev) (!INTEL_INFO(dev)->is_valleyview && IS_GEN8(dev))
> #define IS_MOBILE(dev) (INTEL_INFO(dev)->is_mobile)
> #define IS_HSW_EARLY_SDV(dev) (IS_HASWELL(dev) && \
> - ((dev)->pdev->device & 0xFF00) == 0x0C00)
> + (INTEL_DEVID(dev) & 0xFF00) == 0x0C00)
> #define IS_BDW_ULT(dev) (IS_BROADWELL(dev) && \
> - (((dev)->pdev->device & 0xf) == 0x2 || \
> - ((dev)->pdev->device & 0xf) == 0x6 || \
> - ((dev)->pdev->device & 0xf) == 0xe))
> + ((INTEL_DEVID(dev) & 0xf) == 0x2 || \
> + (INTEL_DEVID(dev) & 0xf) == 0x6 || \
> + (INTEL_DEVID(dev) & 0xf) == 0xe))
> #define IS_HSW_ULT(dev) (IS_HASWELL(dev) && \
> - ((dev)->pdev->device & 0xFF00) == 0x0A00)
> + (INTEL_DEVID(dev) & 0xFF00) == 0x0A00)
> #define IS_ULT(dev) (IS_HSW_ULT(dev) || IS_BDW_ULT(dev))
> #define IS_HSW_GT3(dev) (IS_HASWELL(dev) && \
> - ((dev)->pdev->device & 0x00F0) == 0x0020)
> + (INTEL_DEVID(dev) & 0x00F0) == 0x0020)
> /* ULX machines are also considered ULT. */
> -#define IS_HSW_ULX(dev) ((dev)->pdev->device == 0x0A0E || \
> - (dev)->pdev->device == 0x0A1E)
> +#define IS_HSW_ULX(dev) (INTEL_DEVID(dev) == 0x0A0E || \
> + INTEL_DEVID(dev) == 0x0A1E)
> #define IS_PRELIMINARY_HW(intel_info) ((intel_info)->is_preliminary)
>
> /*
> --
> 2.1.0.rc1
>
> _______________________________________________
> 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] 8+ messages in thread
* Re: [PATCH 2/2] drm/i915: Agnostic INTEL_INFO
2014-08-09 18:18 ` [PATCH 2/2] drm/i915: Agnostic INTEL_INFO Chris Wilson
@ 2014-08-11 10:14 ` Jani Nikula
2014-08-11 10:26 ` Chris Wilson
0 siblings, 1 reply; 8+ messages in thread
From: Jani Nikula @ 2014-08-11 10:14 UTC (permalink / raw)
To: Chris Wilson, intel-gfx
On Sat, 09 Aug 2014, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> Adapt the macro so that we can pass either the struct drm_device or the
> struct drm_i915_private pointers and get the answer we want. Over time,
> my plan is to convert all users over to using drm_i915_private and so
> trimming down the pointer dance. Having spent a few hours chasing that
> goal and achieved over 8k of object code saving, it appears to be a
> worthwhile target. This interim macro allows us to slowly convert over.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
> drivers/gpu/drm/i915/i915_dma.c | 3 +++
> drivers/gpu/drm/i915/i915_drv.h | 3 ++-
> 2 files changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index 39988940d468..49149406fcd8 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -1585,6 +1585,9 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
> if (!drm_core_check_feature(dev, DRIVER_MODESET) && !dev->agp)
> return -EINVAL;
>
> + /* For the ugly agnostic INTEL_INFO macro */
> + BUILD_BUG_ON(sizeof(*dev_priv) == sizeof(*dev));
> +
> dev_priv = kzalloc(sizeof(*dev_priv), GFP_KERNEL);
> if (dev_priv == NULL)
> return -ENOMEM;
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 0aef763ffa75..e66465430bdc 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2063,7 +2063,8 @@ struct drm_i915_cmd_table {
> int count;
> };
>
> -#define INTEL_INFO(p) (&to_i915(p)->info)
> +#define __I915__(p) ((sizeof(*(p)) == sizeof(struct drm_i915_private)) ? (struct drm_i915_private *)(p) : to_i915((struct drm_device *)p))
The explicit casts make me uncomfortable. Indeed, aren't they completely
unnecessary? If the sizeof matches, p is expected to be struct
drm_i915_private *, and if not, it's expected to be struct drm_device *,
right?
#define __I915__(p) ((sizeof(*(p)) == sizeof(struct drm_i915_private)) ? (p) : to_i915(p))
Otherwise you won't even get a compiler warning if you pass a random
pointer to this macro.
BR,
Jani.
> +#define INTEL_INFO(p) (&__I915__(p)->info)
> #define INTEL_DEVID(p) (INTEL_INFO(p)->device_id)
>
> #define IS_I830(dev) (INTEL_DEVID(dev) == 0x3577)
> --
> 2.1.0.rc1
>
> _______________________________________________
> 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] 8+ messages in thread
* Re: [PATCH 2/2] drm/i915: Agnostic INTEL_INFO
2014-08-11 10:14 ` Jani Nikula
@ 2014-08-11 10:26 ` Chris Wilson
2014-08-11 11:25 ` Daniel Vetter
0 siblings, 1 reply; 8+ messages in thread
From: Chris Wilson @ 2014-08-11 10:26 UTC (permalink / raw)
To: Jani Nikula; +Cc: intel-gfx
On Mon, Aug 11, 2014 at 01:14:50PM +0300, Jani Nikula wrote:
> On Sat, 09 Aug 2014, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > Adapt the macro so that we can pass either the struct drm_device or the
> > struct drm_i915_private pointers and get the answer we want. Over time,
> > my plan is to convert all users over to using drm_i915_private and so
> > trimming down the pointer dance. Having spent a few hours chasing that
> > goal and achieved over 8k of object code saving, it appears to be a
> > worthwhile target. This interim macro allows us to slowly convert over.
> >
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > ---
> > drivers/gpu/drm/i915/i915_dma.c | 3 +++
> > drivers/gpu/drm/i915/i915_drv.h | 3 ++-
> > 2 files changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> > index 39988940d468..49149406fcd8 100644
> > --- a/drivers/gpu/drm/i915/i915_dma.c
> > +++ b/drivers/gpu/drm/i915/i915_dma.c
> > @@ -1585,6 +1585,9 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
> > if (!drm_core_check_feature(dev, DRIVER_MODESET) && !dev->agp)
> > return -EINVAL;
> >
> > + /* For the ugly agnostic INTEL_INFO macro */
> > + BUILD_BUG_ON(sizeof(*dev_priv) == sizeof(*dev));
> > +
> > dev_priv = kzalloc(sizeof(*dev_priv), GFP_KERNEL);
> > if (dev_priv == NULL)
> > return -ENOMEM;
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 0aef763ffa75..e66465430bdc 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -2063,7 +2063,8 @@ struct drm_i915_cmd_table {
> > int count;
> > };
> >
> > -#define INTEL_INFO(p) (&to_i915(p)->info)
> > +#define __I915__(p) ((sizeof(*(p)) == sizeof(struct drm_i915_private)) ? (struct drm_i915_private *)(p) : to_i915((struct drm_device *)p))
>
> The explicit casts make me uncomfortable. Indeed, aren't they completely
> unnecessary? If the sizeof matches, p is expected to be struct
> drm_i915_private *, and if not, it's expected to be struct drm_device *,
> right?
Yes, killing the typesafety is bad. Sadly it is to quiesce the compiler
as the type-mismatch warnings are generated before it does the dead code
elimination removing the constant ?: expression. Too bad we couldn't
simply write typeof(*p) == typeof(T).
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] drm/i915: Agnostic INTEL_INFO
2014-08-11 10:26 ` Chris Wilson
@ 2014-08-11 11:25 ` Daniel Vetter
2014-08-11 11:30 ` Chris Wilson
0 siblings, 1 reply; 8+ messages in thread
From: Daniel Vetter @ 2014-08-11 11:25 UTC (permalink / raw)
To: Chris Wilson, Jani Nikula, intel-gfx
On Mon, Aug 11, 2014 at 11:26:07AM +0100, Chris Wilson wrote:
> On Mon, Aug 11, 2014 at 01:14:50PM +0300, Jani Nikula wrote:
> > On Sat, 09 Aug 2014, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > > Adapt the macro so that we can pass either the struct drm_device or the
> > > struct drm_i915_private pointers and get the answer we want. Over time,
> > > my plan is to convert all users over to using drm_i915_private and so
> > > trimming down the pointer dance. Having spent a few hours chasing that
> > > goal and achieved over 8k of object code saving, it appears to be a
> > > worthwhile target. This interim macro allows us to slowly convert over.
> > >
> > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > ---
> > > drivers/gpu/drm/i915/i915_dma.c | 3 +++
> > > drivers/gpu/drm/i915/i915_drv.h | 3 ++-
> > > 2 files changed, 5 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> > > index 39988940d468..49149406fcd8 100644
> > > --- a/drivers/gpu/drm/i915/i915_dma.c
> > > +++ b/drivers/gpu/drm/i915/i915_dma.c
> > > @@ -1585,6 +1585,9 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
> > > if (!drm_core_check_feature(dev, DRIVER_MODESET) && !dev->agp)
> > > return -EINVAL;
> > >
> > > + /* For the ugly agnostic INTEL_INFO macro */
> > > + BUILD_BUG_ON(sizeof(*dev_priv) == sizeof(*dev));
> > > +
> > > dev_priv = kzalloc(sizeof(*dev_priv), GFP_KERNEL);
> > > if (dev_priv == NULL)
> > > return -ENOMEM;
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > > index 0aef763ffa75..e66465430bdc 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > @@ -2063,7 +2063,8 @@ struct drm_i915_cmd_table {
> > > int count;
> > > };
> > >
> > > -#define INTEL_INFO(p) (&to_i915(p)->info)
> > > +#define __I915__(p) ((sizeof(*(p)) == sizeof(struct drm_i915_private)) ? (struct drm_i915_private *)(p) : to_i915((struct drm_device *)p))
> >
> > The explicit casts make me uncomfortable. Indeed, aren't they completely
> > unnecessary? If the sizeof matches, p is expected to be struct
> > drm_i915_private *, and if not, it's expected to be struct drm_device *,
> > right?
>
> Yes, killing the typesafety is bad. Sadly it is to quiesce the compiler
> as the type-mismatch warnings are generated before it does the dead code
> elimination removing the constant ?: expression. Too bad we couldn't
> simply write typeof(*p) == typeof(T).
Do we need the 2nd cast for (struct drm_device *)? If we could drop that
we'd have about as much type-safety as before, presuming no one manages to
matche our drm_i915_private exactly in size.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] drm/i915: Copy PCI device id into the device info block
2014-08-11 10:05 ` [PATCH 1/2] drm/i915: Copy PCI device id into the device info block Jani Nikula
@ 2014-08-11 11:26 ` Daniel Vetter
0 siblings, 0 replies; 8+ messages in thread
From: Daniel Vetter @ 2014-08-11 11:26 UTC (permalink / raw)
To: Jani Nikula; +Cc: intel-gfx
On Mon, Aug 11, 2014 at 01:05:03PM +0300, Jani Nikula wrote:
> On Sat, 09 Aug 2014, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > This is so that we can make the drm_i915_private->info always the
> > preferred source for chipset type and feature queries.
>
> Reviewed-by: Jani Nikula <jani.nikula@intel.com>
Queued for -next, thanks for the patch.
-Daniel
>
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > ---
> > drivers/gpu/drm/i915/i915_dma.c | 5 +++--
> > drivers/gpu/drm/i915/i915_drv.h | 50 +++++++++++++++++++++--------------------
> > 2 files changed, 29 insertions(+), 26 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> > index 96bf11e7a011..39988940d468 100644
> > --- a/drivers/gpu/drm/i915/i915_dma.c
> > +++ b/drivers/gpu/drm/i915/i915_dma.c
> > @@ -1592,9 +1592,10 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
> > dev->dev_private = dev_priv;
> > dev_priv->dev = dev;
> >
> > - /* copy initial configuration to dev_priv->info */
> > + /* Setup the write-once "constant" device info */
> > device_info = (struct intel_device_info *)&dev_priv->info;
> > - *device_info = *info;
> > + memcpy(device_info, info, sizeof(dev_priv->info));
> > + device_info->device_id = dev->pdev->device;
> >
> > spin_lock_init(&dev_priv->irq_lock);
> > spin_lock_init(&dev_priv->gpu_error.lock);
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index b3d42b085790..0aef763ffa75 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -563,6 +563,7 @@ struct intel_uncore {
> >
> > struct intel_device_info {
> > u32 display_mmio_offset;
> > + u16 device_id;
> > u8 num_pipes:3;
> > u8 num_sprites[I915_MAX_PIPES];
> > u8 gen;
> > @@ -2062,51 +2063,52 @@ struct drm_i915_cmd_table {
> > int count;
> > };
> >
> > -#define INTEL_INFO(dev) (&to_i915(dev)->info)
> > +#define INTEL_INFO(p) (&to_i915(p)->info)
> > +#define INTEL_DEVID(p) (INTEL_INFO(p)->device_id)
> >
> > -#define IS_I830(dev) ((dev)->pdev->device == 0x3577)
> > -#define IS_845G(dev) ((dev)->pdev->device == 0x2562)
> > +#define IS_I830(dev) (INTEL_DEVID(dev) == 0x3577)
> > +#define IS_845G(dev) (INTEL_DEVID(dev) == 0x2562)
> > #define IS_I85X(dev) (INTEL_INFO(dev)->is_i85x)
> > -#define IS_I865G(dev) ((dev)->pdev->device == 0x2572)
> > +#define IS_I865G(dev) (INTEL_DEVID(dev) == 0x2572)
> > #define IS_I915G(dev) (INTEL_INFO(dev)->is_i915g)
> > -#define IS_I915GM(dev) ((dev)->pdev->device == 0x2592)
> > -#define IS_I945G(dev) ((dev)->pdev->device == 0x2772)
> > +#define IS_I915GM(dev) (INTEL_DEVID(dev) == 0x2592)
> > +#define IS_I945G(dev) (INTEL_DEVID(dev) == 0x2772)
> > #define IS_I945GM(dev) (INTEL_INFO(dev)->is_i945gm)
> > #define IS_BROADWATER(dev) (INTEL_INFO(dev)->is_broadwater)
> > #define IS_CRESTLINE(dev) (INTEL_INFO(dev)->is_crestline)
> > -#define IS_GM45(dev) ((dev)->pdev->device == 0x2A42)
> > +#define IS_GM45(dev) (INTEL_DEVID(dev) == 0x2A42)
> > #define IS_G4X(dev) (INTEL_INFO(dev)->is_g4x)
> > -#define IS_PINEVIEW_G(dev) ((dev)->pdev->device == 0xa001)
> > -#define IS_PINEVIEW_M(dev) ((dev)->pdev->device == 0xa011)
> > +#define IS_PINEVIEW_G(dev) (INTEL_DEVID(dev) == 0xa001)
> > +#define IS_PINEVIEW_M(dev) (INTEL_DEVID(dev) == 0xa011)
> > #define IS_PINEVIEW(dev) (INTEL_INFO(dev)->is_pineview)
> > #define IS_G33(dev) (INTEL_INFO(dev)->is_g33)
> > -#define IS_IRONLAKE_M(dev) ((dev)->pdev->device == 0x0046)
> > +#define IS_IRONLAKE_M(dev) (INTEL_DEVID(dev) == 0x0046)
> > #define IS_IVYBRIDGE(dev) (INTEL_INFO(dev)->is_ivybridge)
> > -#define IS_IVB_GT1(dev) ((dev)->pdev->device == 0x0156 || \
> > - (dev)->pdev->device == 0x0152 || \
> > - (dev)->pdev->device == 0x015a)
> > -#define IS_SNB_GT1(dev) ((dev)->pdev->device == 0x0102 || \
> > - (dev)->pdev->device == 0x0106 || \
> > - (dev)->pdev->device == 0x010A)
> > +#define IS_IVB_GT1(dev) (INTEL_DEVID(dev) == 0x0156 || \
> > + INTEL_DEVID(dev) == 0x0152 || \
> > + INTEL_DEVID(dev) == 0x015a)
> > +#define IS_SNB_GT1(dev) (INTEL_DEVID(dev) == 0x0102 || \
> > + INTEL_DEVID(dev) == 0x0106 || \
> > + INTEL_DEVID(dev) == 0x010A)
> > #define IS_VALLEYVIEW(dev) (INTEL_INFO(dev)->is_valleyview)
> > #define IS_CHERRYVIEW(dev) (INTEL_INFO(dev)->is_valleyview && IS_GEN8(dev))
> > #define IS_HASWELL(dev) (INTEL_INFO(dev)->is_haswell)
> > #define IS_BROADWELL(dev) (!INTEL_INFO(dev)->is_valleyview && IS_GEN8(dev))
> > #define IS_MOBILE(dev) (INTEL_INFO(dev)->is_mobile)
> > #define IS_HSW_EARLY_SDV(dev) (IS_HASWELL(dev) && \
> > - ((dev)->pdev->device & 0xFF00) == 0x0C00)
> > + (INTEL_DEVID(dev) & 0xFF00) == 0x0C00)
> > #define IS_BDW_ULT(dev) (IS_BROADWELL(dev) && \
> > - (((dev)->pdev->device & 0xf) == 0x2 || \
> > - ((dev)->pdev->device & 0xf) == 0x6 || \
> > - ((dev)->pdev->device & 0xf) == 0xe))
> > + ((INTEL_DEVID(dev) & 0xf) == 0x2 || \
> > + (INTEL_DEVID(dev) & 0xf) == 0x6 || \
> > + (INTEL_DEVID(dev) & 0xf) == 0xe))
> > #define IS_HSW_ULT(dev) (IS_HASWELL(dev) && \
> > - ((dev)->pdev->device & 0xFF00) == 0x0A00)
> > + (INTEL_DEVID(dev) & 0xFF00) == 0x0A00)
> > #define IS_ULT(dev) (IS_HSW_ULT(dev) || IS_BDW_ULT(dev))
> > #define IS_HSW_GT3(dev) (IS_HASWELL(dev) && \
> > - ((dev)->pdev->device & 0x00F0) == 0x0020)
> > + (INTEL_DEVID(dev) & 0x00F0) == 0x0020)
> > /* ULX machines are also considered ULT. */
> > -#define IS_HSW_ULX(dev) ((dev)->pdev->device == 0x0A0E || \
> > - (dev)->pdev->device == 0x0A1E)
> > +#define IS_HSW_ULX(dev) (INTEL_DEVID(dev) == 0x0A0E || \
> > + INTEL_DEVID(dev) == 0x0A1E)
> > #define IS_PRELIMINARY_HW(intel_info) ((intel_info)->is_preliminary)
> >
> > /*
> > --
> > 2.1.0.rc1
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Jani Nikula, Intel Open Source Technology Center
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] drm/i915: Agnostic INTEL_INFO
2014-08-11 11:25 ` Daniel Vetter
@ 2014-08-11 11:30 ` Chris Wilson
0 siblings, 0 replies; 8+ messages in thread
From: Chris Wilson @ 2014-08-11 11:30 UTC (permalink / raw)
To: Daniel Vetter; +Cc: intel-gfx
On Mon, Aug 11, 2014 at 01:25:49PM +0200, Daniel Vetter wrote:
> On Mon, Aug 11, 2014 at 11:26:07AM +0100, Chris Wilson wrote:
> > On Mon, Aug 11, 2014 at 01:14:50PM +0300, Jani Nikula wrote:
> > > On Sat, 09 Aug 2014, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > > > Adapt the macro so that we can pass either the struct drm_device or the
> > > > struct drm_i915_private pointers and get the answer we want. Over time,
> > > > my plan is to convert all users over to using drm_i915_private and so
> > > > trimming down the pointer dance. Having spent a few hours chasing that
> > > > goal and achieved over 8k of object code saving, it appears to be a
> > > > worthwhile target. This interim macro allows us to slowly convert over.
> > > >
> > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > > ---
> > > > drivers/gpu/drm/i915/i915_dma.c | 3 +++
> > > > drivers/gpu/drm/i915/i915_drv.h | 3 ++-
> > > > 2 files changed, 5 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> > > > index 39988940d468..49149406fcd8 100644
> > > > --- a/drivers/gpu/drm/i915/i915_dma.c
> > > > +++ b/drivers/gpu/drm/i915/i915_dma.c
> > > > @@ -1585,6 +1585,9 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
> > > > if (!drm_core_check_feature(dev, DRIVER_MODESET) && !dev->agp)
> > > > return -EINVAL;
> > > >
> > > > + /* For the ugly agnostic INTEL_INFO macro */
> > > > + BUILD_BUG_ON(sizeof(*dev_priv) == sizeof(*dev));
> > > > +
> > > > dev_priv = kzalloc(sizeof(*dev_priv), GFP_KERNEL);
> > > > if (dev_priv == NULL)
> > > > return -ENOMEM;
> > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > > > index 0aef763ffa75..e66465430bdc 100644
> > > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > > @@ -2063,7 +2063,8 @@ struct drm_i915_cmd_table {
> > > > int count;
> > > > };
> > > >
> > > > -#define INTEL_INFO(p) (&to_i915(p)->info)
> > > > +#define __I915__(p) ((sizeof(*(p)) == sizeof(struct drm_i915_private)) ? (struct drm_i915_private *)(p) : to_i915((struct drm_device *)p))
> > >
> > > The explicit casts make me uncomfortable. Indeed, aren't they completely
> > > unnecessary? If the sizeof matches, p is expected to be struct
> > > drm_i915_private *, and if not, it's expected to be struct drm_device *,
> > > right?
> >
> > Yes, killing the typesafety is bad. Sadly it is to quiesce the compiler
> > as the type-mismatch warnings are generated before it does the dead code
> > elimination removing the constant ?: expression. Too bad we couldn't
> > simply write typeof(*p) == typeof(T).
>
> Do we need the 2nd cast for (struct drm_device *)? If we could drop that
> we'd have about as much type-safety as before, presuming no one manages to
> matche our drm_i915_private exactly in size.
The compiler is happy with that. I can't remember exactly why I added
it first time, it certainly wasn't for its looks.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2014-08-11 11:30 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-08-09 18:18 [PATCH 1/2] drm/i915: Copy PCI device id into the device info block Chris Wilson
2014-08-09 18:18 ` [PATCH 2/2] drm/i915: Agnostic INTEL_INFO Chris Wilson
2014-08-11 10:14 ` Jani Nikula
2014-08-11 10:26 ` Chris Wilson
2014-08-11 11:25 ` Daniel Vetter
2014-08-11 11:30 ` Chris Wilson
2014-08-11 10:05 ` [PATCH 1/2] drm/i915: Copy PCI device id into the device info block Jani Nikula
2014-08-11 11:26 ` Daniel Vetter
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.