Re: [PATCH] drm/i915: clarify IS_GEN vs IS_<product> usage

[Chris Wilson <chris@chris-wilson.co.uk>]

On Thu,  5 May 2011 15:16:45 -0700, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> We generally use the gen number to indicate the generation of the render
> portion of the chip.  In some cases this isn't the same as the display
> generation (as in the case of G33 and GMA500).  So codify the de facto
> usage by converting some IS_GEN checks into product specific checks for
> display related differences.  (Note this makes me wonder about our G33
> watermark handling; shouldn't it be like 965 not 945?  I don't have one
> to test with...).

As far as I've been able to tell, the current code works... So it can't
be too far wrong, and I don't recall any mention of deviations in the gen3
docs.

Whilst you are in the vicinity, does it make sense to rename info->gen to
info->render (or info->render_gen)?

> @@ -230,6 +231,7 @@ struct intel_device_info {
>  	u8 is_broadwater : 1;
>  	u8 is_crestline : 1;
>  	u8 is_ivybridge : 1;
> +	u8 is_sandybridge : 1;
>  	u8 has_fbc : 1;
>  	u8 has_pipe_cxsr : 1;
>  	u8 has_hotplug : 1;

What's the sort order here? ;-)

> @@ -915,10 +917,13 @@ enum intel_chip_family {
>  #define IS_845G(dev)		((dev)->pci_device == 0x2562)
>  #define IS_I85X(dev)		(INTEL_INFO(dev)->is_i85x)
>  #define IS_I865G(dev)		((dev)->pci_device == 0x2572)
> +#define IS_I8XX(dev)		(INTEL_INFO(dev)->is_i8xx)
>  #define IS_I915G(dev)		(INTEL_INFO(dev)->is_i915g)
>  #define IS_I915GM(dev)		((dev)->pci_device == 0x2592)
> +#define IS_I915(dev)		(IS_I915G(dev) || IS_I915GM(dev))
>  #define IS_I945G(dev)		((dev)->pci_device == 0x2772)
>  #define IS_I945GM(dev)		(INTEL_INFO(dev)->is_i945gm)
> +#define IS_I945(dev)		(IS_I945G(dev) || IS_I945GM(dev))
>  #define IS_BROADWATER(dev)	(INTEL_INFO(dev)->is_broadwater)
>  #define IS_CRESTLINE(dev)	(INTEL_INFO(dev)->is_crestline)
>  #define IS_GM45(dev)		((dev)->pci_device == 0x2A42)
> @@ -927,8 +932,10 @@ enum intel_chip_family {
>  #define IS_PINEVIEW_M(dev)	((dev)->pci_device == 0xa011)
>  #define IS_PINEVIEW(dev)	(INTEL_INFO(dev)->is_pineview)
>  #define IS_G33(dev)		(INTEL_INFO(dev)->is_g33)
> +#define IS_I915_DISPLAY(dev)	(IS_I915(dev) || IS_I945(dev) || IS_G33(dev))

Not more nested predicates that mix capability bits and devids - just add
a new bit for the display engine! IS_I915_DISPLAY() is a good name, and
makes me want IS_I830_DISPLAY rather than IS_I8XX. And I think you've just
set a new precedent to distinguish the display engines from the render
engine from the chipsets. Don't let me stop you from completing the job ;)
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre