Re: [PATCH 03/11] drm/simple-kms-helper: Add drm_simple_connector_create()

[Daniel Vetter <daniel@ffwll.ch>]

On Sun, Jan 20, 2019 at 12:43:10PM +0100, Noralf Trønnes wrote:
> This adds a function that creates a simple connector that has only one
> static mode. Additionally add a helper to set &drm_mode_config width
> and height from the static mode.
> 
> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> ---
>  drivers/gpu/drm/drm_simple_kms_helper.c | 122 ++++++++++++++++++++++++
>  include/drm/drm_simple_kms_helper.h     |   6 ++
>  2 files changed, 128 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_simple_kms_helper.c b/drivers/gpu/drm/drm_simple_kms_helper.c
> index 917812448d1b..ca29975afefe 100644
> --- a/drivers/gpu/drm/drm_simple_kms_helper.c
> +++ b/drivers/gpu/drm/drm_simple_kms_helper.c
> @@ -11,6 +11,8 @@
>  #include <drm/drm_atomic.h>
>  #include <drm/drm_atomic_helper.h>
>  #include <drm/drm_crtc_helper.h>
> +#include <drm/drm_device.h>
> +#include <drm/drm_modes.h>
>  #include <drm/drm_plane_helper.h>
>  #include <drm/drm_simple_kms_helper.h>
>  #include <linux/slab.h>
> @@ -299,4 +301,124 @@ int drm_simple_display_pipe_init(struct drm_device *dev,
>  }
>  EXPORT_SYMBOL(drm_simple_display_pipe_init);
>  
> +static const struct drm_connector_helper_funcs drm_simple_connector_hfuncs = {
> +	/* dummy for the atomic helper */
> +};
> +
> +static int drm_simple_connector_fill_modes(struct drm_connector *connector,
> +					   uint32_t maxX, uint32_t maxY)
> +{
> +	return 1;
> +}
> +
> +static void drm_simple_connector_destroy(struct drm_connector *connector)
> +{
> +	drm_connector_cleanup(connector);
> +	kfree(connector);
> +}
> +
> +static const struct drm_connector_funcs drm_simple_connector_funcs = {
> +	.reset = drm_atomic_helper_connector_reset,
> +	.fill_modes = drm_simple_connector_fill_modes,
> +	.destroy = drm_simple_connector_destroy,
> +	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
> +	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
> +};
> +
> +/**
> + * drm_simple_connector_create - Create a connector with one static mode
> + * @dev: DRM device
> + * @connector_type: Connector type
> + * @mode: Supported display mode
> + * @rotation: Initial @mode rotation in degrees

We have rotation properties for this, pls don't use degress here.

Also would be good to wire this up with the rotation property Hans added
recently, for pre-rotated screens (see the kerneldoc for "panel
orientation" and drm_connector_init_panel_orientation_property()). So that
userspace knows how to rotate its rendering to make everything look
correct in the end again.


> + *
> + * This function creates a &drm_connector that has one fixed &drm_display_mode
> + * which will be rotated according to @rotation.

From a functionality pov this is very close to a panel wrapped into a
bridge. I think it would be good to differentiate a bit between these two
cases more. After all there was a really long discussion about how the
panel stuff does or does not exactly fit for tinydrm, would be good to
summarize this here and at least point at drm_panel_bridge_add().

Also would be good to explain in the overview DOC comment that this is a
fully independent part of the simple helpers, and drivers can use one or
the other.
-Daniel

> + *
> + * Returns:
> + * Pointer to connector on success, or ERR_PTR on failure.
> + */
> +struct drm_connector *
> +drm_simple_connector_create(struct drm_device *dev, int connector_type,
> +			    const struct drm_display_mode *mode,
> +			    unsigned int rotation)
> +{
> +	struct drm_display_mode *mode_dup = NULL;
> +	struct drm_connector *connector;
> +	int ret;
> +
> +	connector = kzalloc(sizeof(*connector), GFP_KERNEL);
> +	if (!connector)
> +		return ERR_PTR(-ENOMEM);
> +
> +	drm_connector_helper_add(connector, &drm_simple_connector_hfuncs);
> +	ret = drm_connector_init(dev, connector, &drm_simple_connector_funcs,
> +				 connector_type);
> +	if (ret)
> +		goto err_free;
> +
> +	connector->status = connector_status_connected;
> +
> +	mode_dup = drm_mode_duplicate(dev, mode);
> +	if (!mode_dup) {
> +		ret = -ENOMEM;
> +		goto err_cleanup;
> +	}
> +
> +	if (rotation == 90 || rotation == 270) {
> +		swap(mode_dup->hdisplay, mode_dup->vdisplay);
> +		swap(mode_dup->hsync_start, mode_dup->vsync_start);
> +		swap(mode_dup->hsync_end, mode_dup->vsync_end);
> +		swap(mode_dup->htotal, mode_dup->vtotal);
> +		swap(mode_dup->width_mm, mode_dup->height_mm);
> +	} else if (rotation != 0 && rotation != 180) {
> +		DRM_ERROR("Illegal rotation value %u\n", rotation);
> +		ret = -EINVAL;
> +		goto err_cleanup;
> +	}
> +
> +	mode_dup->type |= DRM_MODE_TYPE_PREFERRED;
> +	if (mode_dup->name[0] == '\0')
> +		drm_mode_set_name(mode_dup);
> +
> +	list_add(&mode_dup->head, &connector->modes);
> +
> +	connector->display_info.width_mm = mode_dup->width_mm;
> +	connector->display_info.height_mm = mode_dup->height_mm;
> +
> +	return connector;
> +
> +err_cleanup:
> +	drm_connector_cleanup(connector);
> +	drm_mode_destroy(dev, mode_dup);
> +err_free:
> +	kfree(connector);
> +
> +	return ERR_PTR(ret);
> +}
> +EXPORT_SYMBOL(drm_simple_connector_create);
> +
> +/**
> + * drm_simple_connector_set_mode_config - Set &drm_mode_config width and height
> + * @connector: Connector
> + *
> + * This function sets the &drm_mode_config min/max width and height based on the
> + * connector fixed display mode.
> + */
> +void drm_simple_connector_set_mode_config(struct drm_connector *connector)
> +{
> +	struct drm_mode_config *mode_config = &connector->dev->mode_config;
> +	struct drm_display_mode *mode;
> +
> +	mode = list_first_entry(&connector->modes, struct drm_display_mode, head);
> +	if (WARN_ON(!mode))
> +		return;
> +
> +	mode_config->min_width = mode->hdisplay;
> +	mode_config->max_width = mode->hdisplay;
> +	mode_config->min_height = mode->vdisplay;
> +	mode_config->max_height = mode->vdisplay;
> +}
> +EXPORT_SYMBOL(drm_simple_connector_set_mode_config);
> +
>  MODULE_LICENSE("GPL");
> diff --git a/include/drm/drm_simple_kms_helper.h b/include/drm/drm_simple_kms_helper.h
> index 451960438a29..ab3d847b7713 100644
> --- a/include/drm/drm_simple_kms_helper.h
> +++ b/include/drm/drm_simple_kms_helper.h
> @@ -182,4 +182,10 @@ int drm_simple_display_pipe_init(struct drm_device *dev,
>  			const uint64_t *format_modifiers,
>  			struct drm_connector *connector);
>  
> +struct drm_connector *
> +drm_simple_connector_create(struct drm_device *dev, int connector_type,
> +			    const struct drm_display_mode *mode,
> +			    unsigned int rotation);
> +void drm_simple_connector_set_mode_config(struct drm_connector *connector);
> +
>  #endif /* __LINUX_DRM_SIMPLE_KMS_HELPER_H */
> -- 
> 2.20.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel