| Message ID | 1445559935-17217-2-git-send-email-matthew.d.roper@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Matt, [auto build test WARNING on drm/drm-next -- if it's inappropriate base, please suggest rules for selecting the more suitable base] url: https://github.com/0day-ci/linux/commits/Matt-Roper/CRTC-background-color-support-for-i915/20151023-082852 reproduce: make htmldocs All warnings (new ones prefixed by >>): drivers/gpu/drm/i915/i915_irq.c:2582: warning: No description found for parameter 'wedged' drivers/gpu/drm/i915/i915_irq.c:2582: warning: No description found for parameter 'fmt' >> include/drm/drm_crtc.h:267: warning: cannot understand function prototype: 'typedef struct ' include/drm/drm_crtc.h:353: warning: No description found for parameter 'mode_blob' include/drm/drm_crtc.h:780: warning: No description found for parameter 'tile_blob_ptr' include/drm/drm_crtc.h:819: warning: No description found for parameter 'rotation' include/drm/drm_crtc.h:915: warning: No description found for parameter 'mutex' include/drm/drm_crtc.h:915: warning: No description found for parameter 'helper_private' include/drm/drm_crtc.h:1203: warning: No description found for parameter 'tile_idr' include/drm/drm_crtc.h:1203: warning: No description found for parameter 'delayed_event' include/drm/drm_crtc.h:1203: warning: No description found for parameter 'edid_property' include/drm/drm_crtc.h:1203: warning: No description found for parameter 'dpms_property' include/drm/drm_crtc.h:1203: warning: No description found for parameter 'path_property' include/drm/drm_crtc.h:1203: warning: No description found for parameter 'tile_property' include/drm/drm_crtc.h:1203: warning: No description found for parameter 'plane_type_property' include/drm/drm_crtc.h:1203: warning: No description found for parameter 'rotation_property' include/drm/drm_crtc.h:1203: warning: No description found for parameter 'prop_src_x' include/drm/drm_crtc.h:1203: warning: No description found for parameter 'prop_src_y' include/drm/drm_crtc.h:1203: warning: No description found for parameter 'prop_src_w' include/drm/drm_crtc.h:1203: warning: No description found for parameter 'prop_src_h' include/drm/drm_crtc.h:1203: warning: No description found for parameter 'prop_crtc_x' include/drm/drm_crtc.h:1203: warning: No description found for parameter 'prop_crtc_y' include/drm/drm_crtc.h:1203: warning: No description found for parameter 'prop_crtc_w' include/drm/drm_crtc.h:1203: warning: No description found for parameter 'prop_crtc_h' include/drm/drm_crtc.h:1203: warning: No description found for parameter 'prop_fb_id' include/drm/drm_crtc.h:1203: warning: No description found for parameter 'prop_crtc_id' include/drm/drm_crtc.h:1203: warning: No description found for parameter 'prop_active' include/drm/drm_crtc.h:1203: warning: No description found for parameter 'prop_mode_id' >> include/drm/drm_crtc.h:1203: warning: No description found for parameter 'prop_background_color' include/drm/drm_crtc.h:1203: warning: No description found for parameter 'dvi_i_subconnector_property' include/drm/drm_crtc.h:1203: warning: No description found for parameter 'dvi_i_select_subconnector_property' include/drm/drm_crtc.h:1203: warning: No description found for parameter 'tv_subconnector_property' include/drm/drm_crtc.h:1203: warning: No description found for parameter 'tv_select_subconnector_property' include/drm/drm_crtc.h:1203: warning: No description found for parameter 'tv_mode_property' include/drm/drm_crtc.h:1203: warning: No description found for parameter 'tv_left_margin_property' include/drm/drm_crtc.h:1203: warning: No description found for parameter 'tv_right_margin_property' include/drm/drm_crtc.h:1203: warning: No description found for parameter 'tv_top_margin_property' include/drm/drm_crtc.h:1203: warning: No description found for parameter 'tv_bottom_margin_property' include/drm/drm_crtc.h:1203: warning: No description found for parameter 'tv_brightness_property' include/drm/drm_crtc.h:1203: warning: No description found for parameter 'tv_contrast_property' include/drm/drm_crtc.h:1203: warning: No description found for parameter 'tv_flicker_reduction_property' include/drm/drm_crtc.h:1203: warning: No description found for parameter 'tv_overscan_property' include/drm/drm_crtc.h:1203: warning: No description found for parameter 'tv_saturation_property' include/drm/drm_crtc.h:1203: warning: No description found for parameter 'tv_hue_property' include/drm/drm_crtc.h:1203: warning: No description found for parameter 'scaling_mode_property' include/drm/drm_crtc.h:1203: warning: No description found for parameter 'aspect_ratio_property' include/drm/drm_crtc.h:1203: warning: No description found for parameter 'dirty_info_property' include/drm/drm_crtc.h:1203: warning: No description found for parameter 'suggested_x_property' include/drm/drm_crtc.h:1203: warning: No description found for parameter 'suggested_y_property' include/drm/drm_crtc.h:1203: warning: No description found for parameter 'allow_fb_modifiers' include/drm/drm_fb_helper.h:148: warning: No description found for parameter 'connector_info' include/drm/drm_dp_helper.h:713: warning: No description found for parameter 'i2c_nack_count' include/drm/drm_dp_helper.h:713: warning: No description found for parameter 'i2c_defer_count' drivers/gpu/drm/drm_dp_mst_topology.c:2227: warning: No description found for parameter 'connector' include/drm/drm_dp_mst_helper.h:97: warning: No description found for parameter 'cached_edid' include/drm/drm_dp_mst_helper.h:471: warning: No description found for parameter 'max_dpcd_transaction_bytes' include/drm/drm_dp_mst_helper.h:471: warning: No description found for parameter 'sink_count' include/drm/drm_dp_mst_helper.h:471: warning: No description found for parameter 'total_slots' include/drm/drm_dp_mst_helper.h:471: warning: No description found for parameter 'avail_slots' include/drm/drm_dp_mst_helper.h:471: warning: No description found for parameter 'total_pbn' include/drm/drm_dp_mst_helper.h:471: warning: No description found for parameter 'qlock' include/drm/drm_dp_mst_helper.h:471: warning: No description found for parameter 'tx_msg_downq' include/drm/drm_dp_mst_helper.h:471: warning: No description found for parameter 'tx_msg_upq' include/drm/drm_dp_mst_helper.h:471: warning: No description found for parameter 'tx_down_in_progress' include/drm/drm_dp_mst_helper.h:471: warning: No description found for parameter 'tx_up_in_progress' include/drm/drm_dp_mst_helper.h:471: warning: No description found for parameter 'payload_lock' include/drm/drm_dp_mst_helper.h:471: warning: No description found for parameter 'proposed_vcpis' include/drm/drm_dp_mst_helper.h:471: warning: No description found for parameter 'payloads' include/drm/drm_dp_mst_helper.h:471: warning: No description found for parameter 'payload_mask' include/drm/drm_dp_mst_helper.h:471: warning: No description found for parameter 'vcpi_mask' include/drm/drm_dp_mst_helper.h:471: warning: No description found for parameter 'tx_waitq' include/drm/drm_dp_mst_helper.h:471: warning: No description found for parameter 'work' include/drm/drm_dp_mst_helper.h:471: warning: No description found for parameter 'tx_work' include/drm/drm_dp_mst_helper.h:471: warning: No description found for parameter 'destroy_connector_list' include/drm/drm_dp_mst_helper.h:471: warning: No description found for parameter 'destroy_connector_lock' include/drm/drm_dp_mst_helper.h:471: warning: No description found for parameter 'destroy_connector_work' drivers/gpu/drm/drm_dp_mst_topology.c:2227: warning: No description found for parameter 'connector' drivers/gpu/drm/drm_irq.c:173: warning: No description found for parameter 'flags' include/drm/drmP.h:168: warning: No description found for parameter 'fmt' include/drm/drmP.h:184: warning: No description found for parameter 'fmt' include/drm/drmP.h:202: warning: No description found for parameter 'fmt' include/drm/drmP.h:247: warning: No description found for parameter 'dev' include/drm/drmP.h:247: warning: No description found for parameter 'data' include/drm/drmP.h:247: warning: No description found for parameter 'file_priv' include/drm/drmP.h:280: warning: No description found for parameter 'ioctl' include/drm/drmP.h:280: warning: No description found for parameter '_func' include/drm/drmP.h:280: warning: No description found for parameter '_flags' include/drm/drmP.h:353: warning: cannot understand function prototype: 'struct drm_lock_data ' include/drm/drmP.h:406: warning: cannot understand function prototype: 'struct drm_driver ' include/drm/drmP.h:656: warning: cannot understand function prototype: 'struct drm_info_list ' include/drm/drmP.h:666: warning: cannot understand function prototype: 'struct drm_info_node ' include/drm/drmP.h:676: warning: cannot understand function prototype: 'struct drm_minor ' include/drm/drmP.h:724: warning: cannot understand function prototype: 'struct drm_device ' drivers/gpu/drm/i915/i915_irq.c:2582: warning: No description found for parameter 'wedged' drivers/gpu/drm/i915/i915_irq.c:2582: warning: No description found for parameter 'fmt' drivers/gpu/drm/i915/i915_irq.c:2582: warning: No description found for parameter 'wedged' drivers/gpu/drm/i915/i915_irq.c:2582: warning: No description found for parameter 'fmt' drivers/gpu/drm/i915/i915_irq.c:2582: warning: No description found for parameter 'wedged' drivers/gpu/drm/i915/i915_irq.c:2582: warning: No description found for parameter 'fmt' drivers/gpu/drm/i915/i915_irq.c:2582: warning: No description found for parameter 'wedged' drivers/gpu/drm/i915/i915_irq.c:2582: warning: No description found for parameter 'fmt' drivers/gpu/drm/i915/i915_gem.c:421: warning: No description found for parameter 'dev' drivers/gpu/drm/i915/i915_gem.c:421: warning: No description found for parameter 'data' drivers/gpu/drm/i915/i915_gem.c:421: warning: No description found for parameter 'file' drivers/gpu/drm/i915/i915_gem.c:686: warning: No description found for parameter 'dev' drivers/gpu/drm/i915/i915_gem.c:686: warning: No description found for parameter 'data' drivers/gpu/drm/i915/i915_gem.c:686: warning: No description found for parameter 'file' drivers/gpu/drm/i915/i915_gem.c:767: warning: No description found for parameter 'dev' drivers/gpu/drm/i915/i915_gem.c:767: warning: No description found for parameter 'obj' drivers/gpu/drm/i915/i915_gem.c:767: warning: No description found for parameter 'args' drivers/gpu/drm/i915/i915_gem.c:767: warning: No description found for parameter 'file' drivers/gpu/drm/i915/i915_gem.c:1029: warning: No description found for parameter 'dev' drivers/gpu/drm/i915/i915_gem.c:1029: warning: No description found for parameter 'data' drivers/gpu/drm/i915/i915_gem.c:1029: warning: No description found for parameter 'file' drivers/gpu/drm/i915/i915_gem.c:1194: warning: No description found for parameter 'rps' drivers/gpu/drm/i915/i915_gem.c:1400: warning: No description found for parameter 'req' drivers/gpu/drm/i915/i915_gem.c:1435: warning: No description found for parameter 'obj' drivers/gpu/drm/i915/i915_gem.c:1435: warning: No description found for parameter 'readonly' drivers/gpu/drm/i915/i915_gem.c:1558: warning: No description found for parameter 'dev' drivers/gpu/drm/i915/i915_gem.c:1558: warning: No description found for parameter 'data' drivers/gpu/drm/i915/i915_gem.c:1558: warning: No description found for parameter 'file' drivers/gpu/drm/i915/i915_gem.c:1621: warning: No description found for parameter 'dev' drivers/gpu/drm/i915/i915_gem.c:1621: warning: No description found for parameter 'data' drivers/gpu/drm/i915/i915_gem.c:1621: warning: No description found for parameter 'file' drivers/gpu/drm/i915/i915_gem.c:1666: warning: No description found for parameter 'dev' drivers/gpu/drm/i915/i915_gem.c:1666: warning: No description found for parameter 'data' drivers/gpu/drm/i915/i915_gem.c:1666: warning: No description found for parameter 'file' drivers/gpu/drm/i915/i915_gem.c:1954: warning: No description found for parameter 'dev' drivers/gpu/drm/i915/i915_gem.c:1954: warning: No description found for parameter 'size' vim +267 include/drm/drm_crtc.h 251 struct drm_atomic_state; 252 253 /** 254 * drm_rgba_t - RGBA property value type 255 * 256 * Structure to abstract away the representation of RGBA values with precision 257 * up to 16 bits per color component. This is primarily intended for use with 258 * DRM properties that need to take a color value since we don't want userspace 259 * to have to worry about the bit layout expected by the underlying hardware. 260 * 261 * We wrap the value in a structure here so that the compiler will flag any 262 * accidental attempts by driver code to directly attempt bitwise operations 263 * that could potentially misinterpret the value. Drivers should instead use 264 * the DRM_RGBA_{RED,GREEN,BLUE,ALPHA}BITS() macros to obtain the component 265 * bits and then build values in the format their hardware expects. 266 */ > 267 typedef struct { 268 uint64_t v; 269 } drm_rgba_t; 270 271 static inline uint16_t 272 drm_rgba_bits(drm_rgba_t c, unsigned compshift, unsigned bits) { 273 uint64_t val; 274 275 if (WARN_ON(bits > 16)) --- 0-DAY kernel test infrastructure Open Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation
On Thu, 22 Oct 2015 17:25:34 -0700 Matt Roper <matthew.d.roper@intel.com> wrote: > To support CRTC background color, we need a way of communicating RGB > color values to the DRM. However there is often a mismatch between how > userspace wants to represent the color value vs how it must be > programmed into the hardware; this mismatch can easily lead to > non-obvious bugs. Let's create a property type that standardizes the > user<->kernel format and add some macros that allow drivers to extract > the bits they care about without having to worry about the internal > representation. > > These properties are still exposed to userspace as range properties, so > the only userspace change we need are some helpers to build RGBA values > appropriately. > > Cc: dri-devel@lists.freedesktop.org > Cc: Chandra Konduru <chandra.konduru@intel.com> > Signed-off-by: Matt Roper <matthew.d.roper@intel.com> > --- > drivers/gpu/drm/drm_atomic.c | 4 ++++ > drivers/gpu/drm/drm_crtc.c | 33 +++++++++++++++++++++++++++++ > include/drm/drm_crtc.h | 49 ++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 86 insertions(+) > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c > index 7bb3845..688ca75 100644 > --- a/drivers/gpu/drm/drm_atomic.c > +++ b/drivers/gpu/drm/drm_atomic.c > @@ -415,6 +415,8 @@ int drm_atomic_crtc_set_property(struct drm_crtc *crtc, > > if (property == config->prop_active) > state->active = val; > + else if (property == config->prop_background_color) > + state->background_color.v = val; > else if (property == config->prop_mode_id) { > struct drm_property_blob *mode = > drm_property_lookup_blob(dev, val); > @@ -450,6 +452,8 @@ drm_atomic_crtc_get_property(struct drm_crtc *crtc, > *val = state->active; > else if (property == config->prop_mode_id) > *val = (state->mode_blob) ? state->mode_blob->base.id : 0; > + else if (property == config->prop_background_color) > + *val = state->background_color.v; > else if (crtc->funcs->atomic_get_property) > return crtc->funcs->atomic_get_property(crtc, state, property, val); > else > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c > index e54660a..1e0dd09 100644 > --- a/drivers/gpu/drm/drm_crtc.c > +++ b/drivers/gpu/drm/drm_crtc.c > @@ -3807,6 +3807,30 @@ struct drm_property *drm_property_create_bool(struct drm_device *dev, int flags, > EXPORT_SYMBOL(drm_property_create_bool); > > /** > + * drm_property_create_rgba - create a new RGBA property type > + * @dev: drm device > + * @flags: flags specifying the property type > + * @name: name of the property > + * > + * This creates a new generic drm property which can then be attached to a drm > + * object with drm_object_attach_property. The returned property object must be > + * freed with drm_property_destroy. > + * > + * Userspace should use the DRM_RGBA() macro to build values with the proper > + * bit layout. > + * > + * Returns: > + * A pointer to the newly created property on success, NULL on failure. > + */ > +struct drm_property *drm_property_create_rgba(struct drm_device *dev, int flags, > + const char *name) > +{ > + return drm_property_create_range(dev, flags, name, > + 0, GENMASK_ULL(63, 0)); > +} > +EXPORT_SYMBOL(drm_property_create_rgba); I'm not sure I understand why drm_property_create_rgba was added. It's not being used. Maybe if the drm_mode_create_background_color_property() below called this instead of create_range directly, we could then change the underlying property type used for rgba without having to locate all properties that are supposed to be of that type. Was that the intention? > + > +/** > * drm_property_add_enum - add a possible value to an enumeration property > * @property: enumeration property to change > * @index: index of the new enumeration > @@ -5778,6 +5802,15 @@ struct drm_property *drm_mode_create_rotation_property(struct drm_device *dev, > } > EXPORT_SYMBOL(drm_mode_create_rotation_property); > > +struct drm_property > +*drm_mode_create_background_color_property(struct drm_device *dev) > +{ > + return drm_property_create_range(dev, DRM_MODE_PROP_ATOMIC, > + "background_color", > + 0, GENMASK_ULL(63, 0)); > +} > +EXPORT_SYMBOL(drm_mode_create_background_color_property); > + > /** > * DOC: Tile group > * > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h > index 3f0c690..64f3e62 100644 > --- a/include/drm/drm_crtc.h > +++ b/include/drm/drm_crtc.h > @@ -251,6 +251,45 @@ struct drm_bridge; > struct drm_atomic_state; > > /** > + * drm_rgba_t - RGBA property value type > + * > + * Structure to abstract away the representation of RGBA values with precision > + * up to 16 bits per color component. This is primarily intended for use with > + * DRM properties that need to take a color value since we don't want userspace > + * to have to worry about the bit layout expected by the underlying hardware. > + * > + * We wrap the value in a structure here so that the compiler will flag any > + * accidental attempts by driver code to directly attempt bitwise operations > + * that could potentially misinterpret the value. Drivers should instead use > + * the DRM_RGBA_{RED,GREEN,BLUE,ALPHA}BITS() macros to obtain the component > + * bits and then build values in the format their hardware expects. > + */ > +typedef struct { > + uint64_t v; > +} drm_rgba_t; > + > +static inline uint16_t > +drm_rgba_bits(drm_rgba_t c, unsigned compshift, unsigned bits) { > + uint64_t val; > + > + if (WARN_ON(bits > 16)) > + bits = 16; > + > + val = c.v & GENMASK_ULL(compshift + 15, compshift); > + return val >> (compshift + 16 - bits); > +} > + > +/* > + * Macros to access the individual color components of an RGBA property value. > + * If the requested number of bits is less than 16, only the most significant > + * bits of the color component will be returned. > + */ > +#define DRM_RGBA_REDBITS(c, bits) drm_rgba_bits(c, 48, bits) > +#define DRM_RGBA_GREENBITS(c, bits) drm_rgba_bits(c, 32, bits) > +#define DRM_RGBA_BLUEBITS(c, bits) drm_rgba_bits(c, 16, bits) > +#define DRM_RGBA_ALPHABITS(c, bits) drm_rgba_bits(c, 0, bits) Do we also need macros that drivers can use to create a RGBA value? I.E. maybe when doing the attach? > + > +/** > * struct drm_crtc_state - mutable CRTC state > * @crtc: backpointer to the CRTC > * @enable: whether the CRTC should be enabled, gates all other state > @@ -264,6 +303,7 @@ struct drm_atomic_state; > * update to ensure framebuffer cleanup isn't done too early > * @adjusted_mode: for use by helpers and drivers to compute adjusted mode timings > * @mode: current mode timings > + * @background_color: background color of regions not covered by planes > * @event: optional pointer to a DRM event to signal upon completion of the > * state update > * @state: backpointer to global drm_atomic_state > @@ -304,6 +344,9 @@ struct drm_crtc_state { > /* blob property to expose current mode to atomic userspace */ > struct drm_property_blob *mode_blob; > > + /* CRTC background color */ > + drm_rgba_t background_color; > + > struct drm_pending_vblank_event *event; > > struct drm_atomic_state *state; > @@ -1115,6 +1158,9 @@ struct drm_mode_config { > struct drm_property *prop_active; > struct drm_property *prop_mode_id; > > + /* crtc properties */ > + struct drm_property *prop_background_color; > + > /* DVI-I properties */ > struct drm_property *dvi_i_subconnector_property; > struct drm_property *dvi_i_select_subconnector_property; > @@ -1365,6 +1411,8 @@ struct drm_property *drm_property_create_object(struct drm_device *dev, > int flags, const char *name, uint32_t type); > struct drm_property *drm_property_create_bool(struct drm_device *dev, int flags, > const char *name); > +struct drm_property *drm_property_create_rgba(struct drm_device *dev, > + int flags, const char *name); > struct drm_property_blob *drm_property_create_blob(struct drm_device *dev, > size_t length, > const void *data); > @@ -1495,6 +1543,7 @@ extern int drm_format_vert_chroma_subsampling(uint32_t format); > extern const char *drm_get_format_name(uint32_t format); > extern struct drm_property *drm_mode_create_rotation_property(struct drm_device *dev, > unsigned int supported_rotations); > +extern struct drm_property *drm_mode_create_background_color_property(struct drm_device *dev); > extern unsigned int drm_rotation_simplify(unsigned int rotation, > unsigned int supported_rotations); >
On Wed, Nov 18, 2015 at 01:35:54PM -0800, Bob Paauwe wrote: > On Thu, 22 Oct 2015 17:25:34 -0700 > Matt Roper <matthew.d.roper@intel.com> wrote: > > > To support CRTC background color, we need a way of communicating RGB > > color values to the DRM. However there is often a mismatch between how > > userspace wants to represent the color value vs how it must be > > programmed into the hardware; this mismatch can easily lead to > > non-obvious bugs. Let's create a property type that standardizes the > > user<->kernel format and add some macros that allow drivers to extract > > the bits they care about without having to worry about the internal > > representation. > > > > These properties are still exposed to userspace as range properties, so > > the only userspace change we need are some helpers to build RGBA values > > appropriately. > > > > Cc: dri-devel@lists.freedesktop.org > > Cc: Chandra Konduru <chandra.konduru@intel.com> > > Signed-off-by: Matt Roper <matthew.d.roper@intel.com> > > --- > > drivers/gpu/drm/drm_atomic.c | 4 ++++ > > drivers/gpu/drm/drm_crtc.c | 33 +++++++++++++++++++++++++++++ > > include/drm/drm_crtc.h | 49 ++++++++++++++++++++++++++++++++++++++++++++ > > 3 files changed, 86 insertions(+) > > > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c > > index 7bb3845..688ca75 100644 > > --- a/drivers/gpu/drm/drm_atomic.c > > +++ b/drivers/gpu/drm/drm_atomic.c > > @@ -415,6 +415,8 @@ int drm_atomic_crtc_set_property(struct drm_crtc *crtc, > > > > if (property == config->prop_active) > > state->active = val; > > + else if (property == config->prop_background_color) > > + state->background_color.v = val; > > else if (property == config->prop_mode_id) { > > struct drm_property_blob *mode = > > drm_property_lookup_blob(dev, val); > > @@ -450,6 +452,8 @@ drm_atomic_crtc_get_property(struct drm_crtc *crtc, > > *val = state->active; > > else if (property == config->prop_mode_id) > > *val = (state->mode_blob) ? state->mode_blob->base.id : 0; > > + else if (property == config->prop_background_color) > > + *val = state->background_color.v; > > else if (crtc->funcs->atomic_get_property) > > return crtc->funcs->atomic_get_property(crtc, state, property, val); > > else > > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c > > index e54660a..1e0dd09 100644 > > --- a/drivers/gpu/drm/drm_crtc.c > > +++ b/drivers/gpu/drm/drm_crtc.c > > @@ -3807,6 +3807,30 @@ struct drm_property *drm_property_create_bool(struct drm_device *dev, int flags, > > EXPORT_SYMBOL(drm_property_create_bool); > > > > /** > > + * drm_property_create_rgba - create a new RGBA property type > > + * @dev: drm device > > + * @flags: flags specifying the property type > > + * @name: name of the property > > + * > > + * This creates a new generic drm property which can then be attached to a drm > > + * object with drm_object_attach_property. The returned property object must be > > + * freed with drm_property_destroy. > > + * > > + * Userspace should use the DRM_RGBA() macro to build values with the proper > > + * bit layout. > > + * > > + * Returns: > > + * A pointer to the newly created property on success, NULL on failure. > > + */ > > +struct drm_property *drm_property_create_rgba(struct drm_device *dev, int flags, > > + const char *name) > > +{ > > + return drm_property_create_range(dev, flags, name, > > + 0, GENMASK_ULL(63, 0)); > > +} > > +EXPORT_SYMBOL(drm_property_create_rgba); > > I'm not sure I understand why drm_property_create_rgba was added. It's > not being used. Maybe if the > drm_mode_create_background_color_property() below called this instead > of create_range directly, we could then change the underlying property > type used for rgba without having to locate all properties that are > supposed to be of that type. Was that the intention? Yeah, I meant to use this for the background_color_property function below, but I guess I forgot to go back and actually make that change. This is more just a helper so that drivers that make driver-specific rgba properties won't have to think about what the internal representation really is (the same way boolean properties are really just a helper for range [0,1] properties). > > > + > > +/** > > * drm_property_add_enum - add a possible value to an enumeration property > > * @property: enumeration property to change > > * @index: index of the new enumeration > > @@ -5778,6 +5802,15 @@ struct drm_property *drm_mode_create_rotation_property(struct drm_device *dev, > > } > > EXPORT_SYMBOL(drm_mode_create_rotation_property); > > > > +struct drm_property > > +*drm_mode_create_background_color_property(struct drm_device *dev) > > +{ > > + return drm_property_create_range(dev, DRM_MODE_PROP_ATOMIC, > > + "background_color", > > + 0, GENMASK_ULL(63, 0)); > > +} > > +EXPORT_SYMBOL(drm_mode_create_background_color_property); > > + > > /** > > * DOC: Tile group > > * > > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h > > index 3f0c690..64f3e62 100644 > > --- a/include/drm/drm_crtc.h > > +++ b/include/drm/drm_crtc.h > > @@ -251,6 +251,45 @@ struct drm_bridge; > > struct drm_atomic_state; > > > > /** > > + * drm_rgba_t - RGBA property value type > > + * > > + * Structure to abstract away the representation of RGBA values with precision > > + * up to 16 bits per color component. This is primarily intended for use with > > + * DRM properties that need to take a color value since we don't want userspace > > + * to have to worry about the bit layout expected by the underlying hardware. > > + * > > + * We wrap the value in a structure here so that the compiler will flag any > > + * accidental attempts by driver code to directly attempt bitwise operations > > + * that could potentially misinterpret the value. Drivers should instead use > > + * the DRM_RGBA_{RED,GREEN,BLUE,ALPHA}BITS() macros to obtain the component > > + * bits and then build values in the format their hardware expects. > > + */ > > +typedef struct { > > + uint64_t v; > > +} drm_rgba_t; > > + > > +static inline uint16_t > > +drm_rgba_bits(drm_rgba_t c, unsigned compshift, unsigned bits) { > > + uint64_t val; > > + > > + if (WARN_ON(bits > 16)) > > + bits = 16; > > + > > + val = c.v & GENMASK_ULL(compshift + 15, compshift); > > + return val >> (compshift + 16 - bits); > > +} > > + > > +/* > > + * Macros to access the individual color components of an RGBA property value. > > + * If the requested number of bits is less than 16, only the most significant > > + * bits of the color component will be returned. > > + */ > > +#define DRM_RGBA_REDBITS(c, bits) drm_rgba_bits(c, 48, bits) > > +#define DRM_RGBA_GREENBITS(c, bits) drm_rgba_bits(c, 32, bits) > > +#define DRM_RGBA_BLUEBITS(c, bits) drm_rgba_bits(c, 16, bits) > > +#define DRM_RGBA_ALPHABITS(c, bits) drm_rgba_bits(c, 0, bits) > > Do we also need macros that drivers can use to create a RGBA value? > I.E. maybe when doing the attach? > Yeah, makes sense; the macro I have in libdrm should also be available in the kernel. I'll add that in the next version. Matt > > + > > +/** > > * struct drm_crtc_state - mutable CRTC state > > * @crtc: backpointer to the CRTC > > * @enable: whether the CRTC should be enabled, gates all other state > > @@ -264,6 +303,7 @@ struct drm_atomic_state; > > * update to ensure framebuffer cleanup isn't done too early > > * @adjusted_mode: for use by helpers and drivers to compute adjusted mode timings > > * @mode: current mode timings > > + * @background_color: background color of regions not covered by planes > > * @event: optional pointer to a DRM event to signal upon completion of the > > * state update > > * @state: backpointer to global drm_atomic_state > > @@ -304,6 +344,9 @@ struct drm_crtc_state { > > /* blob property to expose current mode to atomic userspace */ > > struct drm_property_blob *mode_blob; > > > > + /* CRTC background color */ > > + drm_rgba_t background_color; > > + > > struct drm_pending_vblank_event *event; > > > > struct drm_atomic_state *state; > > @@ -1115,6 +1158,9 @@ struct drm_mode_config { > > struct drm_property *prop_active; > > struct drm_property *prop_mode_id; > > > > + /* crtc properties */ > > + struct drm_property *prop_background_color; > > + > > /* DVI-I properties */ > > struct drm_property *dvi_i_subconnector_property; > > struct drm_property *dvi_i_select_subconnector_property; > > @@ -1365,6 +1411,8 @@ struct drm_property *drm_property_create_object(struct drm_device *dev, > > int flags, const char *name, uint32_t type); > > struct drm_property *drm_property_create_bool(struct drm_device *dev, int flags, > > const char *name); > > +struct drm_property *drm_property_create_rgba(struct drm_device *dev, > > + int flags, const char *name); > > struct drm_property_blob *drm_property_create_blob(struct drm_device *dev, > > size_t length, > > const void *data); > > @@ -1495,6 +1543,7 @@ extern int drm_format_vert_chroma_subsampling(uint32_t format); > > extern const char *drm_get_format_name(uint32_t format); > > extern struct drm_property *drm_mode_create_rotation_property(struct drm_device *dev, > > unsigned int supported_rotations); > > +extern struct drm_property *drm_mode_create_background_color_property(struct drm_device *dev); > > extern unsigned int drm_rotation_simplify(unsigned int rotation, > > unsigned int supported_rotations); > > > > > > -- > -- > Bob Paauwe > Bob.J.Paauwe@intel.com > IOTG / PED Software Organization > Intel Corp. Folsom, CA > (916) 356-6193 >
Hi Matt, On 23 October 2015 at 01:25, Matt Roper <matthew.d.roper@intel.com> wrote: > --- a/include/drm/drm_crtc.h > +++ b/include/drm/drm_crtc.h > +typedef struct { > + uint64_t v; > +} drm_rgba_t; > + Humble request - please don't add typedefs. The drm subsystem (barring legacy core and certain drivers) is relatively clean of them. The extra "struct" is not that much to type :-) Thanks Emil
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 7bb3845..688ca75 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -415,6 +415,8 @@ int drm_atomic_crtc_set_property(struct drm_crtc *crtc, if (property == config->prop_active) state->active = val; + else if (property == config->prop_background_color) + state->background_color.v = val; else if (property == config->prop_mode_id) { struct drm_property_blob *mode = drm_property_lookup_blob(dev, val); @@ -450,6 +452,8 @@ drm_atomic_crtc_get_property(struct drm_crtc *crtc, *val = state->active; else if (property == config->prop_mode_id) *val = (state->mode_blob) ? state->mode_blob->base.id : 0; + else if (property == config->prop_background_color) + *val = state->background_color.v; else if (crtc->funcs->atomic_get_property) return crtc->funcs->atomic_get_property(crtc, state, property, val); else diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index e54660a..1e0dd09 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -3807,6 +3807,30 @@ struct drm_property *drm_property_create_bool(struct drm_device *dev, int flags, EXPORT_SYMBOL(drm_property_create_bool); /** + * drm_property_create_rgba - create a new RGBA property type + * @dev: drm device + * @flags: flags specifying the property type + * @name: name of the property + * + * This creates a new generic drm property which can then be attached to a drm + * object with drm_object_attach_property. The returned property object must be + * freed with drm_property_destroy. + * + * Userspace should use the DRM_RGBA() macro to build values with the proper + * bit layout. + * + * Returns: + * A pointer to the newly created property on success, NULL on failure. + */ +struct drm_property *drm_property_create_rgba(struct drm_device *dev, int flags, + const char *name) +{ + return drm_property_create_range(dev, flags, name, + 0, GENMASK_ULL(63, 0)); +} +EXPORT_SYMBOL(drm_property_create_rgba); + +/** * drm_property_add_enum - add a possible value to an enumeration property * @property: enumeration property to change * @index: index of the new enumeration @@ -5778,6 +5802,15 @@ struct drm_property *drm_mode_create_rotation_property(struct drm_device *dev, } EXPORT_SYMBOL(drm_mode_create_rotation_property); +struct drm_property +*drm_mode_create_background_color_property(struct drm_device *dev) +{ + return drm_property_create_range(dev, DRM_MODE_PROP_ATOMIC, + "background_color", + 0, GENMASK_ULL(63, 0)); +} +EXPORT_SYMBOL(drm_mode_create_background_color_property); + /** * DOC: Tile group * diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 3f0c690..64f3e62 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -251,6 +251,45 @@ struct drm_bridge; struct drm_atomic_state; /** + * drm_rgba_t - RGBA property value type + * + * Structure to abstract away the representation of RGBA values with precision + * up to 16 bits per color component. This is primarily intended for use with + * DRM properties that need to take a color value since we don't want userspace + * to have to worry about the bit layout expected by the underlying hardware. + * + * We wrap the value in a structure here so that the compiler will flag any + * accidental attempts by driver code to directly attempt bitwise operations + * that could potentially misinterpret the value. Drivers should instead use + * the DRM_RGBA_{RED,GREEN,BLUE,ALPHA}BITS() macros to obtain the component + * bits and then build values in the format their hardware expects. + */ +typedef struct { + uint64_t v; +} drm_rgba_t; + +static inline uint16_t +drm_rgba_bits(drm_rgba_t c, unsigned compshift, unsigned bits) { + uint64_t val; + + if (WARN_ON(bits > 16)) + bits = 16; + + val = c.v & GENMASK_ULL(compshift + 15, compshift); + return val >> (compshift + 16 - bits); +} + +/* + * Macros to access the individual color components of an RGBA property value. + * If the requested number of bits is less than 16, only the most significant + * bits of the color component will be returned. + */ +#define DRM_RGBA_REDBITS(c, bits) drm_rgba_bits(c, 48, bits) +#define DRM_RGBA_GREENBITS(c, bits) drm_rgba_bits(c, 32, bits) +#define DRM_RGBA_BLUEBITS(c, bits) drm_rgba_bits(c, 16, bits) +#define DRM_RGBA_ALPHABITS(c, bits) drm_rgba_bits(c, 0, bits) + +/** * struct drm_crtc_state - mutable CRTC state * @crtc: backpointer to the CRTC * @enable: whether the CRTC should be enabled, gates all other state @@ -264,6 +303,7 @@ struct drm_atomic_state; * update to ensure framebuffer cleanup isn't done too early * @adjusted_mode: for use by helpers and drivers to compute adjusted mode timings * @mode: current mode timings + * @background_color: background color of regions not covered by planes * @event: optional pointer to a DRM event to signal upon completion of the * state update * @state: backpointer to global drm_atomic_state @@ -304,6 +344,9 @@ struct drm_crtc_state { /* blob property to expose current mode to atomic userspace */ struct drm_property_blob *mode_blob; + /* CRTC background color */ + drm_rgba_t background_color; + struct drm_pending_vblank_event *event; struct drm_atomic_state *state; @@ -1115,6 +1158,9 @@ struct drm_mode_config { struct drm_property *prop_active; struct drm_property *prop_mode_id; + /* crtc properties */ + struct drm_property *prop_background_color; + /* DVI-I properties */ struct drm_property *dvi_i_subconnector_property; struct drm_property *dvi_i_select_subconnector_property; @@ -1365,6 +1411,8 @@ struct drm_property *drm_property_create_object(struct drm_device *dev, int flags, const char *name, uint32_t type); struct drm_property *drm_property_create_bool(struct drm_device *dev, int flags, const char *name); +struct drm_property *drm_property_create_rgba(struct drm_device *dev, + int flags, const char *name); struct drm_property_blob *drm_property_create_blob(struct drm_device *dev, size_t length, const void *data); @@ -1495,6 +1543,7 @@ extern int drm_format_vert_chroma_subsampling(uint32_t format); extern const char *drm_get_format_name(uint32_t format); extern struct drm_property *drm_mode_create_rotation_property(struct drm_device *dev, unsigned int supported_rotations); +extern struct drm_property *drm_mode_create_background_color_property(struct drm_device *dev); extern unsigned int drm_rotation_simplify(unsigned int rotation, unsigned int supported_rotations);
To support CRTC background color, we need a way of communicating RGB color values to the DRM. However there is often a mismatch between how userspace wants to represent the color value vs how it must be programmed into the hardware; this mismatch can easily lead to non-obvious bugs. Let's create a property type that standardizes the user<->kernel format and add some macros that allow drivers to extract the bits they care about without having to worry about the internal representation. These properties are still exposed to userspace as range properties, so the only userspace change we need are some helpers to build RGBA values appropriately. Cc: dri-devel@lists.freedesktop.org Cc: Chandra Konduru <chandra.konduru@intel.com> Signed-off-by: Matt Roper <matthew.d.roper@intel.com> --- drivers/gpu/drm/drm_atomic.c | 4 ++++ drivers/gpu/drm/drm_crtc.c | 33 +++++++++++++++++++++++++++++ include/drm/drm_crtc.h | 49 ++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 86 insertions(+)