Re: [PATCH 36/37] drm: Add mode_config .get_format_info() hook

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Ville,

Thank you for the patch.

On Friday 18 Nov 2016 21:53:12 ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Allow drivers to return a custom drm_format_info structure for special
> fb layouts. We'll use this for the compression control surface in i915.
> 
> Cc: Ben Widawsky <ben@bwidawsk.net>
> Cc: intel-gfx@lists.freedesktop.org
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/drm_fb_cma_helper.c  |  2 +-
>  drivers/gpu/drm/drm_fourcc.c         | 25 +++++++++++++++++++++++++
>  drivers/gpu/drm/drm_framebuffer.c    |  9 +++++++--
>  drivers/gpu/drm/drm_modeset_helper.c |  2 +-
>  include/drm/drm_fourcc.h             |  6 ++++++
>  include/drm/drm_mode_config.h        | 15 +++++++++++++++
>  6 files changed, 55 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c
> b/drivers/gpu/drm/drm_fb_cma_helper.c index aab4465307ed..d7f8876cf5e9
> 100644
> --- a/drivers/gpu/drm/drm_fb_cma_helper.c
> +++ b/drivers/gpu/drm/drm_fb_cma_helper.c
> @@ -186,7 +186,7 @@ struct drm_framebuffer
> *drm_fb_cma_create_with_funcs(struct drm_device *dev, int ret;
>  	int i;
> 
> -	info = drm_format_info(mode_cmd->pixel_format);
> +	info = drm_get_format_info(dev, mode_cmd);
>  	if (!info)
>  		return ERR_PTR(-EINVAL);
> 
> diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c
> index 90d2cc8da8eb..7cfaee689f0c 100644
> --- a/drivers/gpu/drm/drm_fourcc.c
> +++ b/drivers/gpu/drm/drm_fourcc.c
> @@ -199,6 +199,31 @@ const struct drm_format_info *drm_format_info(u32
> format) EXPORT_SYMBOL(drm_format_info);
> 
>  /**
> + * drm_format_info - query information for a given framebuffer
> configuration

I assume you meant drm_get_format_info()

> + * @dev: DRM device

Do we need the dev pointer ?

> + * @mode_cmd: metadata from the userspace fb creation request
> + *
> + * Returns:
> + * The instance of struct drm_format_info that describes the pixel format,
> or
> + * NULL if the format is unsupported.

It would be useful to document how this function differs from 
drm_format_info(). I also wonder whether it would make sense to completely 
replace drm_format_info() to avoid keeping two separate but very similar 
functions.

> + */
> +const struct drm_format_info *
> +drm_get_format_info(struct drm_device *dev,
> +		    const struct drm_mode_fb_cmd2 *mode_cmd)
> +{
> +	const struct drm_format_info *info = NULL;
> +
> +	if (dev->mode_config.funcs->get_format_info)
> +		info = dev->mode_config.funcs->get_format_info(dev, mode_cmd);
> +
> +	if (!info)
> +		info = drm_format_info(mode_cmd->pixel_format);
> +
> +	return info;
> +}
> +EXPORT_SYMBOL(drm_get_format_info);
> +
> +/**
>   * drm_format_num_planes - get the number of planes for format
>   * @format: pixel format (DRM_FORMAT_*)
>   *
> diff --git a/drivers/gpu/drm/drm_framebuffer.c
> b/drivers/gpu/drm/drm_framebuffer.c index 94ddab41f24f..292930a5dcc2 100644
> --- a/drivers/gpu/drm/drm_framebuffer.c
> +++ b/drivers/gpu/drm/drm_framebuffer.c
> @@ -126,11 +126,13 @@ int drm_mode_addfb(struct drm_device *dev,
>  	return 0;
>  }
> 
> -static int framebuffer_check(const struct drm_mode_fb_cmd2 *r)
> +static int framebuffer_check(struct drm_device *dev,
> +			     const struct drm_mode_fb_cmd2 *r)
>  {
>  	const struct drm_format_info *info;
>  	int i;
> 
> +	/* check if the format is supported at all */
>  	info = __drm_format_info(r->pixel_format & ~DRM_FORMAT_BIG_ENDIAN);
>  	if (!info) {
>  		struct drm_format_name_buf format_name;
> @@ -140,6 +142,9 @@ static int framebuffer_check(const struct
> drm_mode_fb_cmd2 *r) return -EINVAL;
>  	}
> 
> +	/* now let the driver pick its own format info */
> +	info = drm_get_format_info(dev, r);
> +
>  	if (r->width == 0 || r->width % info->hsub) {
>  		DRM_DEBUG_KMS("bad framebuffer width %u\n", r->width);
>  		return -EINVAL;
> @@ -263,7 +268,7 @@ drm_internal_framebuffer_create(struct drm_device *dev,
>  		return ERR_PTR(-EINVAL);
>  	}
> 
> -	ret = framebuffer_check(r);
> +	ret = framebuffer_check(dev, r);
>  	if (ret)
>  		return ERR_PTR(ret);
> 
> diff --git a/drivers/gpu/drm/drm_modeset_helper.c
> b/drivers/gpu/drm/drm_modeset_helper.c index 5b051859b8d3..f78df06a940d
> 100644
> --- a/drivers/gpu/drm/drm_modeset_helper.c
> +++ b/drivers/gpu/drm/drm_modeset_helper.c
> @@ -75,7 +75,7 @@ void drm_helper_mode_fill_fb_struct(struct drm_device
> *dev, int i;
> 
>  	fb->dev = dev;
> -	fb->format = drm_format_info(mode_cmd->pixel_format);
> +	fb->format = drm_get_format_info(dev, mode_cmd);
>  	fb->width = mode_cmd->width;
>  	fb->height = mode_cmd->height;
>  	for (i = 0; i < 4; i++) {
> diff --git a/include/drm/drm_fourcc.h b/include/drm/drm_fourcc.h
> index fcc08da850c8..6942e84b6edd 100644
> --- a/include/drm/drm_fourcc.h
> +++ b/include/drm/drm_fourcc.h
> @@ -25,6 +25,9 @@
>  #include <linux/types.h>
>  #include <uapi/drm/drm_fourcc.h>
> 
> +struct drm_device;
> +struct drm_mode_fb_cmd2;
> +
>  /**
>   * struct drm_format_info - information about a DRM format
>   * @format: 4CC format identifier (DRM_FORMAT_*)
> @@ -55,6 +58,9 @@ struct drm_format_name_buf {
> 
>  const struct drm_format_info *__drm_format_info(u32 format);
>  const struct drm_format_info *drm_format_info(u32 format);
> +const struct drm_format_info *
> +drm_get_format_info(struct drm_device *dev,
> +		    const struct drm_mode_fb_cmd2 *mode_cmd);
>  uint32_t drm_mode_legacy_fb_format(uint32_t bpp, uint32_t depth);
>  int drm_format_num_planes(uint32_t format);
>  int drm_format_plane_cpp(uint32_t format, int plane);
> diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
> index bf9991b20611..80bb7e49bf8e 100644
> --- a/include/drm/drm_mode_config.h
> +++ b/include/drm/drm_mode_config.h
> @@ -34,6 +34,7 @@ struct drm_file;
>  struct drm_device;
>  struct drm_atomic_state;
>  struct drm_mode_fb_cmd2;
> +struct drm_format_info;
> 
>  /**
>   * struct drm_mode_config_funcs - basic driver provided mode setting
> functions @@ -70,6 +71,20 @@ struct drm_mode_config_funcs {
>  					     const struct drm_mode_fb_cmd2 
*mode_cmd);
> 
>  	/**
> +	 * @get_format_info:
> +	 *
> +	 * Allows a driver to return custom format information for special
> +	 * fb layouts (eg. ones with auxiliary compresssion control planes).
> +	 *
> +	 * RETURNS:
> +	 *
> +	 * The format information specific to the given fb metadata, or
> +	 * NULL if none is found.
> +	 */
> +	const struct drm_format_info *(*get_format_info)(struct drm_device 
*dev,
> +							 const struct 
drm_mode_fb_cmd2 *mode_cmd);
> +
> +	/**
>  	 * @output_poll_changed:
>  	 *
>  	 * Callback used by helpers to inform the driver of output 
configuration

-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel