| Message ID | 1444418952-5671-10-git-send-email-shashank.sharma@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Shashank, On 9 October 2015 at 20:28, Shashank Sharma <shashank.sharma@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. Regards, Emil
Regards Shashank On 10/10/2015 4:17 AM, Emil Velikov wrote: > Hi Shashank, > > On 9 October 2015 at 20:28, Shashank Sharma <shashank.sharma@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. > Regards, > Emil >
On 10 October 2015 at 05:55, Sharma, Shashank <shashank.sharma@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@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
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@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@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. > 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 >
diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile index 44d290a..56caf9e 100644 --- a/drivers/gpu/drm/i915/Makefile +++ b/drivers/gpu/drm/i915/Makefile @@ -64,7 +64,8 @@ i915-y += intel_audio.o \ intel_overlay.o \ intel_psr.o \ intel_sideband.o \ - intel_sprite.o + intel_sprite.o \ + intel_color_manager.o i915-$(CONFIG_ACPI) += intel_acpi.o intel_opregion.o i915-$(CONFIG_DRM_FBDEV_EMULATION) += intel_fbdev.o diff --git a/drivers/gpu/drm/i915/intel_color_manager.c b/drivers/gpu/drm/i915/intel_color_manager.c new file mode 100644 index 0000000..7357d99 --- /dev/null +++ b/drivers/gpu/drm/i915/intel_color_manager.c @@ -0,0 +1,33 @@ +/* + * Copyright © 2015 Intel Corporation + * + * Permission is hereby granted, free of charge, to any person obtaining a + * copy of this software and associated documentation files (the "Software"), + * to deal in the Software without restriction, including without limitation + * the rights to use, copy, modify, merge, publish, distribute, sublicense, + * and/or sell copies of the Software, and to permit persons to whom the + * Software is furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice (including the next + * paragraph) shall be included in all copies or substantial portions of the + * Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS + * IN THE SOFTWARE. + * + * Authors: + * Shashank Sharma <shashank.sharma@intel.com> + * Kausal Malladi <Kausal.Malladi@intel.com> + */ + +#include "intel_color_manager.h" + +void intel_attach_color_properties_to_crtc(struct drm_device *dev, + struct drm_mode_object *mode_obj) +{ +} diff --git a/drivers/gpu/drm/i915/intel_color_manager.h b/drivers/gpu/drm/i915/intel_color_manager.h new file mode 100644 index 0000000..eec52a7 --- /dev/null +++ b/drivers/gpu/drm/i915/intel_color_manager.h @@ -0,0 +1,50 @@ +/* + * Copyright © 2015 Intel Corporation + * + * Permission is hereby granted, free of charge, to any person obtaining a + * copy of this software and associated documentation files (the "Software"), + * to deal in the Software without restriction, including without limitation + * the rights to use, copy, modify, merge, publish, distribute, sublicense, + * and/or sell copies of the Software, and to permit persons to whom the + * Software is furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice (including the next + * paragraph) shall be included in all copies or substantial portions of the + * Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS + * IN THE SOFTWARE. + * + * Authors: + * Shashank Sharma <shashank.sharma@intel.com> + * Kausal Malladi <Kausal.Malladi@intel.com> + */ +#include <drm/drmP.h> +#include <drm/drm_crtc_helper.h> +#include "i915_drv.h" + +/* 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)