| Message ID | 1470750091-16627-20-git-send-email-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Aug 9, 2016 at 9:41 AM, 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. > > 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 +++--------- > drivers/gpu/drm/gma500/cdv_intel_lvds.c | 8 ----- > include/drm/drm_connector.h | 63 ++++++++++++++++++++++++++++++--- > include/drm/drm_crtc.h | 4 --- > 6 files changed, 97 insertions(+), 69 deletions(-) > > diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c > index cedc3bc4e10b..9e93e0d13117 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 69b5626141f5..7bcfb3466ba9 100644 > --- a/drivers/gpu/drm/drm_crtc.c > +++ b/drivers/gpu/drm/drm_crtc.c > @@ -391,40 +391,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 553390fae803..a4a576424a66 100644 > --- a/drivers/gpu/drm/drm_edid.c > +++ b/drivers/gpu/drm/drm_edid.c > @@ -3727,14 +3727,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. > */ > @@ -3828,16 +3821,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) > @@ -4044,7 +4027,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/drivers/gpu/drm/gma500/cdv_intel_lvds.c b/drivers/gpu/drm/gma500/cdv_intel_lvds.c > index 38dc89083148..ea733ab5b1e0 100644 > --- a/drivers/gpu/drm/gma500/cdv_intel_lvds.c > +++ b/drivers/gpu/drm/gma500/cdv_intel_lvds.c > @@ -415,14 +415,6 @@ static int cdv_intel_lvds_get_modes(struct drm_connector *connector) > if (ret) > return ret; > > - /* Didn't get an EDID, so > - * Set wide sync ranges so we get all modes > - * handed to valid_mode for checking > - */ > - connector->display_info.min_vfreq = 0; > - connector->display_info.max_vfreq = 200; > - connector->display_info.min_hfreq = 0; > - connector->display_info.max_hfreq = 200; Wrong patch? > if (mode_dev->panel_fixed_mode != NULL) { > struct drm_display_mode *mode = > drm_mode_duplicate(dev, mode_dev->panel_fixed_mode); > diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h > index fa42d2071bfb..11c13c0be5d4 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 width in mm. *height > + */ > 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 accae9ed6cd7..787d2943a920 100644 > --- a/include/drm/drm_crtc.h > +++ b/include/drm/drm_crtc.h > @@ -2194,10 +2194,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 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c index cedc3bc4e10b..9e93e0d13117 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 69b5626141f5..7bcfb3466ba9 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -391,40 +391,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 553390fae803..a4a576424a66 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -3727,14 +3727,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. */ @@ -3828,16 +3821,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) @@ -4044,7 +4027,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/drivers/gpu/drm/gma500/cdv_intel_lvds.c b/drivers/gpu/drm/gma500/cdv_intel_lvds.c index 38dc89083148..ea733ab5b1e0 100644 --- a/drivers/gpu/drm/gma500/cdv_intel_lvds.c +++ b/drivers/gpu/drm/gma500/cdv_intel_lvds.c @@ -415,14 +415,6 @@ static int cdv_intel_lvds_get_modes(struct drm_connector *connector) if (ret) return ret; - /* Didn't get an EDID, so - * Set wide sync ranges so we get all modes - * handed to valid_mode for checking - */ - connector->display_info.min_vfreq = 0; - connector->display_info.max_vfreq = 200; - connector->display_info.min_hfreq = 0; - connector->display_info.max_hfreq = 200; if (mode_dev->panel_fixed_mode != NULL) { struct drm_display_mode *mode = drm_mode_duplicate(dev, mode_dev->panel_fixed_mode); diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h index fa42d2071bfb..11c13c0be5d4 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 width 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 accae9ed6cd7..787d2943a920 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -2194,10 +2194,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. 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 +++--------- drivers/gpu/drm/gma500/cdv_intel_lvds.c | 8 ----- include/drm/drm_connector.h | 63 ++++++++++++++++++++++++++++++--- include/drm/drm_crtc.h | 4 --- 6 files changed, 97 insertions(+), 69 deletions(-)