Re: [PATCH 1/2] drm/ttm: add a way to bo_wait for either the last read or last write

[Thomas Hellstrom <thomas@shipmail.org>]

On 10/24/2011 06:42 PM, Marek Olšák wrote:
> On Sat, Oct 8, 2011 at 1:32 PM, Thomas Hellstrom<thomas@shipmail.org>  wrote:
>    
>> On 10/08/2011 01:27 PM, Ville Syrjälä wrote:
>>      
>>> On Sat, Oct 08, 2011 at 01:10:13PM +0200, Thomas Hellstrom wrote:
>>>
>>>        
>>>> On 10/08/2011 12:26 PM, Ville Syrjälä wrote:
>>>>
>>>>          
>>>>> On Fri, Oct 07, 2011 at 10:58:13AM +0200, Thomas Hellstrom wrote:
>>>>>
>>>>>
>>>>>            
>>>>>> Oh, and one more style comment below:
>>>>>>
>>>>>> On 08/07/2011 10:39 PM, Marek Olšák wrote:
>>>>>>
>>>>>>
>>>>>>              
>>>>>>> +enum ttm_buffer_usage {
>>>>>>> +    TTM_USAGE_READ = 1,
>>>>>>> +    TTM_USAGE_WRITE = 2,
>>>>>>> +    TTM_USAGE_READWRITE = TTM_USAGE_READ | TTM_USAGE_WRITE
>>>>>>> +};
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>                
>>>>>> Please don't use enums for bit operations.
>>>>>>
>>>>>>
>>>>>>              
>>>>> Now I'm curious. Why not?
>>>>>
>>>>>
>>>>>
>>>>>            
>>>> Because it's inconsistent with how flags are defined in the rest of the
>>>> TTM module.
>>>>
>>>>          
>>> Ah OK. I was wondering if there's some subtle technical issue involved.
>>> I've recently gotten to the habit of using enums for pretty much all
>>> constants. Just easier on the eye IMHO, and avoids cpp output from
>>> looking like number soup.
>>>
>>>
>>>        
>> Yes, there are a number of advantages, including symbolic debugger output.
>> If we had flag enums that enumerated 1, 2, 4, 8 etc. I'd feel motivated to
>> move
>> all TTM definitions over.
>>      
> I don't think that how it is enumerated matters in any way. What I
> like about enums, besides what has already been mentioned, is that it
> adds a self-documentation in the code. Compare:
>
> void ttm_set_bo_flags(unsigned flags);
>
> And:
>
> void ttm_set_bo_flags(enum ttm_bo_flags flags);
>
> The latter is way easier to understand for somebody who doesn't know
> the code and wants to implement his first patch. With the latter, it's
> clear at first glance what are the valid values for "flags", because
> you can just search for "enum ttm_bo_flags".
>
> I will change the enum to defines for the sake of following your code
> style convention, but it's an unreasonable convention to say the
> least.
>
> Marek
>    

I'm not going to argue against this, because you're probably right.

The important thing is that we get the fix in with or without enums.

Thanks,
Thomas



_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel