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

["Noralf Trønnes" <noralf@tronnes.org>]

[cc:Hans]

Den 21.01.2019 10.22, skrev Daniel Vetter:
> 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.
> 

This rotation represents the way the display is mounted in the casing.
It is configured using a DT property:

Documentation/devicetree/bindings/display/panel/panel.txt:
- rotation:	Display rotation in degrees counter clockwise (0,90,180,270)

In the driver I set up a display mode which is rotated to match how it's
mounted:

static const struct drm_display_mode mode = {
	DRM_SIMPLE_MODE(320, 240, 58, 43),
};

	device_property_read_u32(dev, "rotation", &rotation);

	connector = drm_simple_connector_create(drm,
DRM_MODE_CONNECTOR_VIRTUAL, &mode, rotation);


The display controller is configured to scan out according to this rotation.

It's important that the display mode matches the case mounting because
of fbdev. fbdev userspace has very limited support for rotation. Most
utilities have no support for it.

The work Hans did seems to only care about fbcon and it doesn't support
90/270 hw rotation.

Noralf.

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