| Message ID | 1471034937-651-19-git-send-email-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Aug 12, 2016 at 4:48 PM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > We seem to have a bit a mess in how to describe the bus formats, with > a multitude of competing ways. Might be best to consolidate it all and > use MEDIA_BUS_FMT_ also for the hdmi color formats and high color > modes. > > Also move all the display_info related functions into drm_connector.c > (there's only one) to group it all together. I did decided against > also moving the edid related display info functions, they seem to fit > better in drm_edid.c. Instead sprinkle a few cross references around. > While at that reduce the kerneldoc for static functions, there's not > point in documenting internals with that much detail really. > > v2: Fix typo and move misplaced hunk (Sean). > > Cc: Sean Paul <seanpaul@chromium.org> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> Reviewed-by: Sean Paul <seanpaul@chromium.org> > --- > drivers/gpu/drm/drm_connector.c | 34 ++++++++++++++++++++++ > drivers/gpu/drm/drm_crtc.c | 34 ---------------------- > drivers/gpu/drm/drm_edid.c | 23 +++------------ > include/drm/drm_connector.h | 63 ++++++++++++++++++++++++++++++++++++++--- > include/drm/drm_crtc.h | 4 --- > 5 files changed, 97 insertions(+), 61 deletions(-) > > diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c > index 6a0551744d13..26bb78c76481 100644 > --- a/drivers/gpu/drm/drm_connector.c > +++ b/drivers/gpu/drm/drm_connector.c > @@ -506,6 +506,40 @@ static const struct drm_prop_enum_list drm_dpms_enum_list[] = { > }; > DRM_ENUM_NAME_FN(drm_get_dpms_name, drm_dpms_enum_list) > > +/** > + * drm_display_info_set_bus_formats - set the supported bus formats > + * @info: display info to store bus formats in > + * @formats: array containing the supported bus formats > + * @num_formats: the number of entries in the fmts array > + * > + * Store the supported bus formats in display info structure. > + * See MEDIA_BUS_FMT_* definitions in include/uapi/linux/media-bus-format.h for > + * a full list of available formats. > + */ > +int drm_display_info_set_bus_formats(struct drm_display_info *info, > + const u32 *formats, > + unsigned int num_formats) > +{ > + u32 *fmts = NULL; > + > + if (!formats && num_formats) > + return -EINVAL; > + > + if (formats && num_formats) { > + fmts = kmemdup(formats, sizeof(*formats) * num_formats, > + GFP_KERNEL); > + if (!fmts) > + return -ENOMEM; > + } > + > + kfree(info->bus_formats); > + info->bus_formats = fmts; > + info->num_bus_formats = num_formats; > + > + return 0; > +} > +EXPORT_SYMBOL(drm_display_info_set_bus_formats); > + > /* Optional connector properties. */ > static const struct drm_prop_enum_list drm_scaling_mode_enum_list[] = { > { DRM_MODE_SCALE_NONE, "None" }, > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c > index 07eba82a9998..d228c41b572b 100644 > --- a/drivers/gpu/drm/drm_crtc.c > +++ b/drivers/gpu/drm/drm_crtc.c > @@ -419,40 +419,6 @@ void drm_crtc_cleanup(struct drm_crtc *crtc) > } > EXPORT_SYMBOL(drm_crtc_cleanup); > > -/** > - * drm_display_info_set_bus_formats - set the supported bus formats > - * @info: display info to store bus formats in > - * @formats: array containing the supported bus formats > - * @num_formats: the number of entries in the fmts array > - * > - * Store the supported bus formats in display info structure. > - * See MEDIA_BUS_FMT_* definitions in include/uapi/linux/media-bus-format.h for > - * a full list of available formats. > - */ > -int drm_display_info_set_bus_formats(struct drm_display_info *info, > - const u32 *formats, > - unsigned int num_formats) > -{ > - u32 *fmts = NULL; > - > - if (!formats && num_formats) > - return -EINVAL; > - > - if (formats && num_formats) { > - fmts = kmemdup(formats, sizeof(*formats) * num_formats, > - GFP_KERNEL); > - if (!fmts) > - return -ENOMEM; > - } > - > - kfree(info->bus_formats); > - info->bus_formats = fmts; > - info->num_bus_formats = num_formats; > - > - return 0; > -} > -EXPORT_SYMBOL(drm_display_info_set_bus_formats); > - > static int drm_encoder_register_all(struct drm_device *dev) > { > struct drm_encoder *encoder; > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c > index c071bd635160..9b1ec64d579a 100644 > --- a/drivers/gpu/drm/drm_edid.c > +++ b/drivers/gpu/drm/drm_edid.c > @@ -3732,14 +3732,7 @@ bool drm_rgb_quant_range_selectable(struct edid *edid) > } > EXPORT_SYMBOL(drm_rgb_quant_range_selectable); > > -/** > - * drm_assign_hdmi_deep_color_info - detect whether monitor supports > - * hdmi deep color modes and update drm_display_info if so. > - * @edid: monitor EDID information > - * @info: Updated with maximum supported deep color bpc and color format > - * if deep color supported. > - * @connector: DRM connector, used only for debug output > - * > +/* > * Parse the CEA extension according to CEA-861-B. > * Return true if HDMI deep color supported, false if not or unknown. > */ > @@ -3833,16 +3826,6 @@ static bool drm_assign_hdmi_deep_color_info(struct edid *edid, > return false; > } > > -/** > - * drm_add_display_info - pull display info out if present > - * @edid: EDID data > - * @info: display info (attached to connector) > - * @connector: connector whose edid is used to build display info > - * > - * Grab any available display info and stuff it into the drm_display_info > - * structure that's part of the connector. Useful for tracking bpp and > - * color spaces. > - */ > static void drm_add_display_info(struct edid *edid, > struct drm_display_info *info, > struct drm_connector *connector) > @@ -4063,7 +4046,9 @@ static int add_displayid_detailed_modes(struct drm_connector *connector, > * @connector: connector we're probing > * @edid: EDID data > * > - * Add the specified modes to the connector's mode list. > + * Add the specified modes to the connector's mode list. Also fills out the > + * &drm_display_info structure in @connector with any information which can be > + * derived from the edid. > * > * Return: The number of modes added or 0 if we couldn't find any. > */ > diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h > index bc88a5575792..f1576db6c044 100644 > --- a/include/drm/drm_connector.h > +++ b/include/drm/drm_connector.h > @@ -84,28 +84,69 @@ enum subpixel_order { > SubPixelNone, > }; > > -/* > - * Describes a given display (e.g. CRT or flat panel) and its limitations. > +/** > + * struct drm_display_info - runtime data about the connected sink > + * > + * Describes a given display (e.g. CRT or flat panel) and its limitations. For > + * fixed display sinks like built-in panels there's not much difference between > + * this and struct &drm_connector. But for sinks with a real cable this > + * structure is meant to describe all the things at the other end of the cable. > + * > + * For sinks which provide an EDID this can be filled out by calling > + * drm_add_edid_modes(). > */ > struct drm_display_info { > + /** > + * @name: Name of the display. > + */ > char name[DRM_DISPLAY_INFO_LEN]; > > - /* Physical size */ > + /** > + * @width_mm: Physical width in mm. > + */ > unsigned int width_mm; > + /** > + * @height_mm: Physical height in mm. > + */ > unsigned int height_mm; > > + /** > + * @pixel_clock: Maximum pixel clock supported by the sink, in units of > + * 100Hz. This mismatches the clok in &drm_display_mode (which is in > + * kHZ), because that's what the EDID uses as base unit. > + */ > unsigned int pixel_clock; > + /** > + * @bpc: Maximum bits per color channel. Used by HDMI and DP outputs. > + */ > unsigned int bpc; > > + /** > + * @subpixel_order: Subpixel order of LCD panels. > + */ > enum subpixel_order subpixel_order; > > #define DRM_COLOR_FORMAT_RGB444 (1<<0) > #define DRM_COLOR_FORMAT_YCRCB444 (1<<1) > #define DRM_COLOR_FORMAT_YCRCB422 (1<<2) > > + /** > + * @color_formats: HDMI Color formats, selects between RGB and YCrCb > + * modes. Used DRM_COLOR_FORMAT\_ defines, which are _not_ the same ones > + * as used to describe the pixel format in framebuffers, and also don't > + * match the formats in @bus_formats which are shared with v4l. > + */ > u32 color_formats; > > + /** > + * @bus_formats: Pixel data format on the wire, somewhat redundant with > + * @color_formats. Array of size @num_bus_formats encoded using > + * MEDIA_BUS_FMT\_ defines shared with v4l and media drivers. > + */ > const u32 *bus_formats; > + /** > + * @num_bus_formats: Size of @bus_formats array. > + */ > unsigned int num_bus_formats; > > #define DRM_BUS_FLAG_DE_LOW (1<<0) > @@ -115,14 +156,28 @@ struct drm_display_info { > /* drive data on neg. edge */ > #define DRM_BUS_FLAG_PIXDATA_NEGEDGE (1<<3) > > + /** > + * @bus_flags: Additional information (like pixel signal polarity) for > + * the pixel data on the bus, using DRM_BUS_FLAGS\_ defines. > + */ > u32 bus_flags; > > - /* Mask of supported hdmi deep color modes */ > + /** > + * @edid_hdmi_dc_modes: Mask of supported hdmi deep color modes. Even > + * more stuff redundant with @bus_formats. > + */ > u8 edid_hdmi_dc_modes; > > + /** > + * @cea_rev: CEA revision of the HDMI sink. > + */ > u8 cea_rev; > }; > > +int drm_display_info_set_bus_formats(struct drm_display_info *info, > + const u32 *formats, > + unsigned int num_formats); > + > /** > * struct drm_connector_state - mutable connector state > * @connector: backpointer to the connector > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h > index e30ea0be6417..3fa0275e509f 100644 > --- a/include/drm/drm_crtc.h > +++ b/include/drm/drm_crtc.h > @@ -2208,10 +2208,6 @@ extern void drm_mode_config_init(struct drm_device *dev); > extern void drm_mode_config_reset(struct drm_device *dev); > extern void drm_mode_config_cleanup(struct drm_device *dev); > > -extern int drm_display_info_set_bus_formats(struct drm_display_info *info, > - const u32 *formats, > - unsigned int num_formats); > - > static inline bool drm_property_type_is(struct drm_property *property, > uint32_t type) > { > -- > 2.8.1 >
diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c index 6a0551744d13..26bb78c76481 100644 --- a/drivers/gpu/drm/drm_connector.c +++ b/drivers/gpu/drm/drm_connector.c @@ -506,6 +506,40 @@ static const struct drm_prop_enum_list drm_dpms_enum_list[] = { }; DRM_ENUM_NAME_FN(drm_get_dpms_name, drm_dpms_enum_list) +/** + * drm_display_info_set_bus_formats - set the supported bus formats + * @info: display info to store bus formats in + * @formats: array containing the supported bus formats + * @num_formats: the number of entries in the fmts array + * + * Store the supported bus formats in display info structure. + * See MEDIA_BUS_FMT_* definitions in include/uapi/linux/media-bus-format.h for + * a full list of available formats. + */ +int drm_display_info_set_bus_formats(struct drm_display_info *info, + const u32 *formats, + unsigned int num_formats) +{ + u32 *fmts = NULL; + + if (!formats && num_formats) + return -EINVAL; + + if (formats && num_formats) { + fmts = kmemdup(formats, sizeof(*formats) * num_formats, + GFP_KERNEL); + if (!fmts) + return -ENOMEM; + } + + kfree(info->bus_formats); + info->bus_formats = fmts; + info->num_bus_formats = num_formats; + + return 0; +} +EXPORT_SYMBOL(drm_display_info_set_bus_formats); + /* Optional connector properties. */ static const struct drm_prop_enum_list drm_scaling_mode_enum_list[] = { { DRM_MODE_SCALE_NONE, "None" }, diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 07eba82a9998..d228c41b572b 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -419,40 +419,6 @@ void drm_crtc_cleanup(struct drm_crtc *crtc) } EXPORT_SYMBOL(drm_crtc_cleanup); -/** - * drm_display_info_set_bus_formats - set the supported bus formats - * @info: display info to store bus formats in - * @formats: array containing the supported bus formats - * @num_formats: the number of entries in the fmts array - * - * Store the supported bus formats in display info structure. - * See MEDIA_BUS_FMT_* definitions in include/uapi/linux/media-bus-format.h for - * a full list of available formats. - */ -int drm_display_info_set_bus_formats(struct drm_display_info *info, - const u32 *formats, - unsigned int num_formats) -{ - u32 *fmts = NULL; - - if (!formats && num_formats) - return -EINVAL; - - if (formats && num_formats) { - fmts = kmemdup(formats, sizeof(*formats) * num_formats, - GFP_KERNEL); - if (!fmts) - return -ENOMEM; - } - - kfree(info->bus_formats); - info->bus_formats = fmts; - info->num_bus_formats = num_formats; - - return 0; -} -EXPORT_SYMBOL(drm_display_info_set_bus_formats); - static int drm_encoder_register_all(struct drm_device *dev) { struct drm_encoder *encoder; diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index c071bd635160..9b1ec64d579a 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -3732,14 +3732,7 @@ bool drm_rgb_quant_range_selectable(struct edid *edid) } EXPORT_SYMBOL(drm_rgb_quant_range_selectable); -/** - * drm_assign_hdmi_deep_color_info - detect whether monitor supports - * hdmi deep color modes and update drm_display_info if so. - * @edid: monitor EDID information - * @info: Updated with maximum supported deep color bpc and color format - * if deep color supported. - * @connector: DRM connector, used only for debug output - * +/* * Parse the CEA extension according to CEA-861-B. * Return true if HDMI deep color supported, false if not or unknown. */ @@ -3833,16 +3826,6 @@ static bool drm_assign_hdmi_deep_color_info(struct edid *edid, return false; } -/** - * drm_add_display_info - pull display info out if present - * @edid: EDID data - * @info: display info (attached to connector) - * @connector: connector whose edid is used to build display info - * - * Grab any available display info and stuff it into the drm_display_info - * structure that's part of the connector. Useful for tracking bpp and - * color spaces. - */ static void drm_add_display_info(struct edid *edid, struct drm_display_info *info, struct drm_connector *connector) @@ -4063,7 +4046,9 @@ static int add_displayid_detailed_modes(struct drm_connector *connector, * @connector: connector we're probing * @edid: EDID data * - * Add the specified modes to the connector's mode list. + * Add the specified modes to the connector's mode list. Also fills out the + * &drm_display_info structure in @connector with any information which can be + * derived from the edid. * * Return: The number of modes added or 0 if we couldn't find any. */ diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h index bc88a5575792..f1576db6c044 100644 --- a/include/drm/drm_connector.h +++ b/include/drm/drm_connector.h @@ -84,28 +84,69 @@ enum subpixel_order { SubPixelNone, }; -/* - * Describes a given display (e.g. CRT or flat panel) and its limitations. +/** + * struct drm_display_info - runtime data about the connected sink + * + * Describes a given display (e.g. CRT or flat panel) and its limitations. For + * fixed display sinks like built-in panels there's not much difference between + * this and struct &drm_connector. But for sinks with a real cable this + * structure is meant to describe all the things at the other end of the cable. + * + * For sinks which provide an EDID this can be filled out by calling + * drm_add_edid_modes(). */ struct drm_display_info { + /** + * @name: Name of the display. + */ char name[DRM_DISPLAY_INFO_LEN]; - /* Physical size */ + /** + * @width_mm: Physical width in mm. + */ unsigned int width_mm; + /** + * @height_mm: Physical height in mm. + */ unsigned int height_mm; + /** + * @pixel_clock: Maximum pixel clock supported by the sink, in units of + * 100Hz. This mismatches the clok in &drm_display_mode (which is in + * kHZ), because that's what the EDID uses as base unit. + */ unsigned int pixel_clock; + /** + * @bpc: Maximum bits per color channel. Used by HDMI and DP outputs. + */ unsigned int bpc; + /** + * @subpixel_order: Subpixel order of LCD panels. + */ enum subpixel_order subpixel_order; #define DRM_COLOR_FORMAT_RGB444 (1<<0) #define DRM_COLOR_FORMAT_YCRCB444 (1<<1) #define DRM_COLOR_FORMAT_YCRCB422 (1<<2) + /** + * @color_formats: HDMI Color formats, selects between RGB and YCrCb + * modes. Used DRM_COLOR_FORMAT\_ defines, which are _not_ the same ones + * as used to describe the pixel format in framebuffers, and also don't + * match the formats in @bus_formats which are shared with v4l. + */ u32 color_formats; + /** + * @bus_formats: Pixel data format on the wire, somewhat redundant with + * @color_formats. Array of size @num_bus_formats encoded using + * MEDIA_BUS_FMT\_ defines shared with v4l and media drivers. + */ const u32 *bus_formats; + /** + * @num_bus_formats: Size of @bus_formats array. + */ unsigned int num_bus_formats; #define DRM_BUS_FLAG_DE_LOW (1<<0) @@ -115,14 +156,28 @@ struct drm_display_info { /* drive data on neg. edge */ #define DRM_BUS_FLAG_PIXDATA_NEGEDGE (1<<3) + /** + * @bus_flags: Additional information (like pixel signal polarity) for + * the pixel data on the bus, using DRM_BUS_FLAGS\_ defines. + */ u32 bus_flags; - /* Mask of supported hdmi deep color modes */ + /** + * @edid_hdmi_dc_modes: Mask of supported hdmi deep color modes. Even + * more stuff redundant with @bus_formats. + */ u8 edid_hdmi_dc_modes; + /** + * @cea_rev: CEA revision of the HDMI sink. + */ u8 cea_rev; }; +int drm_display_info_set_bus_formats(struct drm_display_info *info, + const u32 *formats, + unsigned int num_formats); + /** * struct drm_connector_state - mutable connector state * @connector: backpointer to the connector diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index e30ea0be6417..3fa0275e509f 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -2208,10 +2208,6 @@ extern void drm_mode_config_init(struct drm_device *dev); extern void drm_mode_config_reset(struct drm_device *dev); extern void drm_mode_config_cleanup(struct drm_device *dev); -extern int drm_display_info_set_bus_formats(struct drm_display_info *info, - const u32 *formats, - unsigned int num_formats); - static inline bool drm_property_type_is(struct drm_property *property, uint32_t type) {
We seem to have a bit a mess in how to describe the bus formats, with a multitude of competing ways. Might be best to consolidate it all and use MEDIA_BUS_FMT_ also for the hdmi color formats and high color modes. Also move all the display_info related functions into drm_connector.c (there's only one) to group it all together. I did decided against also moving the edid related display info functions, they seem to fit better in drm_edid.c. Instead sprinkle a few cross references around. While at that reduce the kerneldoc for static functions, there's not point in documenting internals with that much detail really. v2: Fix typo and move misplaced hunk (Sean). Cc: Sean Paul <seanpaul@chromium.org> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> --- drivers/gpu/drm/drm_connector.c | 34 ++++++++++++++++++++++ drivers/gpu/drm/drm_crtc.c | 34 ---------------------- drivers/gpu/drm/drm_edid.c | 23 +++------------ include/drm/drm_connector.h | 63 ++++++++++++++++++++++++++++++++++++++--- include/drm/drm_crtc.h | 4 --- 5 files changed, 97 insertions(+), 61 deletions(-)