[Mesa-dev] [PATCH 12/29] i965: Move device quirks to brw_device_info.

[Ian Romanick]

On 09/30/2013 10:11 PM, Kenneth Graunke wrote:
> On 09/30/2013 05:16 PM, Ian Romanick wrote:
>> On 09/27/2013 04:45 PM, Kenneth Graunke wrote:
>>> Signed-off-by: Kenneth Graunke <kenneth at whitecape.org>
>>> ---
>>>  src/mesa/drivers/dri/i965/brw_context.c     | 10 +++-------
>>>  src/mesa/drivers/dri/i965/brw_device_info.c |  9 ++++++++-
>>>  src/mesa/drivers/dri/i965/brw_device_info.h | 16 ++++++++++++++++
>>>  3 files changed, 27 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/src/mesa/drivers/dri/i965/brw_context.c b/src/mesa/drivers/dri/i965/brw_context.c
>>> index 266f504..53073aa 100644
>>> --- a/src/mesa/drivers/dri/i965/brw_context.c
>>> +++ b/src/mesa/drivers/dri/i965/brw_context.c
>>> @@ -362,6 +362,9 @@ brwCreateContext(int api,
>>>     brw->has_llc = devinfo->has_llc;
>>>     brw->has_hiz = devinfo->has_hiz_and_separate_stencil;
>>>     brw->has_separate_stencil = devinfo->has_hiz_and_separate_stencil;
>>> +   brw->has_negative_rhw_bug = devinfo->has_negative_rhw_bug;
>>> +   brw->needs_unlit_centroid_workaround =
>>> +      devinfo->needs_unlit_centroid_workaround;
>>>  
>>>     brw->must_use_separate_stencil = screen->hw_must_use_separate_stencil;
>>>     brw->has_swizzling = screen->hw_has_swizzling;
>>> @@ -451,13 +454,6 @@ brwCreateContext(int api,
>>>     if (brw->gen == 6)
>>>        brw->urb.gen6_gs_previously_active = false;
>>>  
>>> -   if (brw->gen == 4 && !brw->is_g4x)
>>> -      brw->has_negative_rhw_bug = true;
>>> -
>>> -   if (brw->gen <= 7) {
>>> -      brw->needs_unlit_centroid_workaround = true;
>>> -   }
>>> -
>>>     brw->prim_restart.in_progress = false;
>>>     brw->prim_restart.enable_cut_index = false;
>>>  
>>> diff --git a/src/mesa/drivers/dri/i965/brw_device_info.c b/src/mesa/drivers/dri/i965/brw_device_info.c
>>> index 7dad8ba8..a215917 100644
>>> --- a/src/mesa/drivers/dri/i965/brw_device_info.c
>>> +++ b/src/mesa/drivers/dri/i965/brw_device_info.c
>>> @@ -27,6 +27,8 @@
>>>  
>>>  static const struct brw_device_info brw_device_info_i965 = {
>>>     .gen = 4,
>>> +   .has_negative_rhw_bug = true,
>>> +   .needs_unlit_centroid_workaround = true,
>>>     .max_vs_threads = 16,
>>>     .max_gs_threads = 2,
>>>     .max_wm_threads = 8 * 4,
>>> @@ -37,6 +39,7 @@ static const struct brw_device_info brw_device_info_i965 = {
>>>  
>>>  static const struct brw_device_info brw_device_info_g4x = {
>>>     .gen = 4,
>>> +   .needs_unlit_centroid_workaround = true,
>>>     .is_g4x = true,
>>>     .max_vs_threads = 32,
>>>     .max_gs_threads = 2,
>>> @@ -48,6 +51,7 @@ static const struct brw_device_info brw_device_info_g4x = {
>>>  
>>>  static const struct brw_device_info brw_device_info_ilk = {
>>>     .gen = 5,
>>> +   .needs_unlit_centroid_workaround = true,
>>>     .max_vs_threads = 72,
>>>     .max_gs_threads = 32,
>>>     .max_wm_threads = 12 * 6,
>>> @@ -61,6 +65,7 @@ static const struct brw_device_info brw_device_info_snb_gt1 = {
>>>     .gt = 2,
>>>     .has_hiz_and_separate_stencil = true,
>>>     .has_llc = true,
>>> +   .needs_unlit_centroid_workaround = true,
>>>     .max_vs_threads = 24,
>>>     .max_gs_threads = 21, /* conservative; 24 if rendering disabled. */
>>>     .max_wm_threads = 40,
>>> @@ -77,6 +82,7 @@ static const struct brw_device_info brw_device_info_snb_gt2 = {
>>>     .gt = 2,
>>>     .has_hiz_and_separate_stencil = true,
>>>     .has_llc = true,
>>> +   .needs_unlit_centroid_workaround = true,
>>>     .max_vs_threads = 60,
>>>     .max_gs_threads = 60,
>>>     .max_wm_threads = 80,
>>> @@ -92,7 +98,8 @@ static const struct brw_device_info brw_device_info_snb_gt2 = {
>>>     .gen = 7,                                        \
>>>     .has_hiz_and_separate_stencil = true,            \
>>>     .must_use_separate_stencil = true,               \
>>> -   .has_llc = true
>>> +   .has_llc = true,                                 \
>>> +   .needs_unlit_centroid_workaround = true
>>>  
>>>  static const struct brw_device_info brw_device_info_ivb_gt1 = {
>>>     GEN7_FEATURES, .is_ivybridge = true, .gt = 1,
>>> diff --git a/src/mesa/drivers/dri/i965/brw_device_info.h b/src/mesa/drivers/dri/i965/brw_device_info.h
>>> index 0f4c282..39f4d57 100644
>>> --- a/src/mesa/drivers/dri/i965/brw_device_info.h
>>> +++ b/src/mesa/drivers/dri/i965/brw_device_info.h
>>> @@ -41,6 +41,22 @@ struct brw_device_info
>>>     bool has_llc;
>>>  
>>>     /**
>>> +    * Quirks:
>>> +    *  @{
>>> +    */
>>> +   bool has_negative_rhw_bug;
>>> +
>>> +   /**
>>> +    * Some versions of Gen hardware don't do centroid interpolation correctly
>>> +    * on unlit pixels, causing incorrect values for derivatives near triangle
>>> +    * edges.  Enabling this flag causes the fragment shader to use
>>> +    * non-centroid interpolation for unlit pixels, at the expense of two extra
>>> +    * fragment shader instructions.
>>> +    */
>>> +   bool needs_unlit_centroid_workaround;
>>> +   /** @} */
>>
>> I believe you want only one * here.
> 
> Huh?  My intention was to associate the comment with the
> needs_unlit_centroid_workaround field in the proper doxygen style.
> 
> Maybe it's not formatted properly for doxygen, and needs a short
> comment/long comment, but...I dunno...

Doxygen just needs the @} to match the @{.  There are a couple places in
mtypes.h that use this.  I usually put the @{ in its own comment, but I
don't think that's strictly necessary.  For example, from mtypes.h:

/**
 * Bitflags for vertex attributes.
 * These are used in bitfields in many places.
 */
/*@{*/
...
/*@}*/

>>> +
>>> +   /**
>>>      * GPU Limits:
>>>      *  @{
>>>      */
>>>