Re: [PATCH 19/20] drm: docume drm_display_info

From: Sean Paul <seanpaul@chromium.org>
To: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Daniel Vetter <daniel.vetter@intel.com>,
	Intel Graphics Development <intel-gfx@lists.freedesktop.org>,
	DRI Development <dri-devel@lists.freedesktop.org>
Subject: Re: [PATCH 19/20] drm: docume drm_display_info
Date: Wed, 10 Aug 2016 11:18:48 -0400	[thread overview]
Message-ID: <CAOw6vbKQS1_Fp+7bT3XLPh=6LastxLCJW2A8U3qo+ZLB8OJRCA@mail.gmail.com> (raw)
In-Reply-To: <1470750091-16627-20-git-send-email-daniel.vetter@ffwll.ch>

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx