Re: [PATCH 09/22] drm/i915: Create color management files

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Thanks for the review Emil.
Please find my comments inline

Regards
Shashank
On 10/13/2015 6:29 PM, Emil Velikov wrote:
On 10 October 2015 at 05:55, Sharma, Shashank <shashank.sharma@xxxxxxxxx> wrote:
On 10/10/2015 4:17 AM, Emil Velikov wrote:

Hi Shashank,

On 9 October 2015 at 20:28, Shashank Sharma <shashank.sharma@xxxxxxxxx>
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.

If you closely observe, what set_bit does is picks up the bit pattern from nth to n+reqd bit, moves it to LSB and returns it. similarly set bits clears the bits, then copy the bit pattern in the respective bits and manipulates the shifts. I could not find any such examples which I can directly use from suggested macros.

Regards,
Emil

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




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux