[Intel-gfx] [PATCH 09/22] drm/i915: Create color management files

[Emil Velikov]

On 10 October 2015 at 05:55, Sharma, Shashank <shashank.sharma at intel.com> wrote:
> On 10/10/2015 4:17 AM, Emil Velikov wrote:
>>
>> Hi Shashank,
>>
>> On 9 October 2015 at 20:28, Shashank Sharma <shashank.sharma at intel.com>
>> wrote:
>> [snip]
>>>
>>> +
>>> +/* Color management bit utilities */
>>> +#define GET_BIT_MASK(n) ((1 << n) - 1)
>>> +
>>> +/* Read bits of a word from bit no. 'start'(lsb) till 'n' bits */
>>> +#define GET_BITS(x, start, nbits) ((x >> start) & GET_BIT_MASK(nbits))
>>> +
>>> +/* Round off by adding 1 to the immediate lower bit */
>>> +#define GET_BITS_ROUNDOFF(x, start, nbits) \
>>> +       ((GET_BITS(x, start, (nbits + 1)) + 1) >> 1)
>>> +
>>> +/* Clear bits of a word from bit no. 'start' till nbits */
>>> +#define CLEAR_BITS(x, start, nbits) ( \
>>> +               x &= ~((GET_BIT_MASK(nbits) << start)))
>>> +
>>> +/* Write bit_pattern of no_bits bits in a target word */
>>> +#define SET_BITS(target, bit_pattern, start_bit, no_bits) \
>>> +               do { \
>>> +                       CLEAR_BITS(target, start_bit, no_bits); \
>>> +                       target |= (bit_pattern << start_bit);  \
>>> +               } while (0)
>>
>> It feels suspicious that the kernel does not have macros for either
>> one of these.
>>
>> I would invite you to take a look at include/linux/bitops.h and other
>> kernel headers.
>>
> Thanks for pointing this out, but if you closely observe, these macros are
> well tested, and created for color management operations, which have
> specific requirements, like:
> - pick 8 bits from 16th bit onwards, make them LSB, and give result:
> GET_BITS
> - take these 8 bits and move to bit 17th of the word, clearing the existing
> ones: SET_BITS
> For core register programming, this was required, so we created it. I would
> still have a look
> at the existing ones which you pointed out to avoid any duplication, if they
> fall directly in the implementation, else I would like to continue with
> these.
Unless I'm missing something, these are generic bit manipulation
macros, are they not ? As such I'd imagine we have some of these
already available, but I cannot say which ones off-hand.

Regards,
Emil