[PATCH 03/11] drm/simple-kms-helper: Add drm_simple_connector_create()
Hans de Goede
hdegoede at redhat.com
Thu Jan 24 14:53:25 UTC 2019
Hi,
On 24-01-19 15:38, Noralf Trønnes wrote:
> [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 at 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.
That sounds wrong, this sounds like you're trying to hide the fact
that the LCD display is not mounted upright from userspace and
transparently deal with this. This is what I wanted to do at
first too, but in the end it turns out that that has a bunch
of issues.
This was extensively discussed and it was decided that this is not
something which we want to do because it gets us into trouble with
things like overlay planes, etc. Also many devices only support
90 / 270 degrees rotation if the framebuffer is tiled in a specific
way, so if we do the rotation transparently, what do we do then
if userspace attaches an incompatible framebuffer to the CRTC?
Instead the decision was made to provide a property on the drm-connector
which tells userspace that the LCD panel is not mounted upright and then
let userspace deal with this.
> 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.
Right, so maybe it is time for userspace to move to the kms API already?
Note that that will not fix things by itself, but at least it makes
the info that the display is not upright available to userspace.
Note I've already patched both plymouth and mutter to use the
drm-connector property for this.
> The work Hans did seems to only care about fbcon
Since fbcon still uses fbdev, and since it already supported
rendering the console rotated I added some glue code to make
it automatically do the right thing for non-upright LCD-panels.
As mentioned I did also fix the userspace kms apps which most
distros use as their default desktop components.
> and it doesn't support 90/270 hw rotation.
Right, because of the framebuffer tiling thing.
So building on top of my work, how this should work is that
the modeline for the display reflect the actual hardware LCD
modeline, so if it is say a 720x1280 portrait screen mounted
in landscape mode, then the modeline will be 720x1280 as that
is what is actually going over the wire to the panel/display.
And the device-tree rotation property can be passed to
drm_connector_init_panel_orientation_property() to let
fbcon and kms API user know they need to render rotated.
Note that kms API users can use hardware rotation to deal
with this if they so desire.
Regards,
Hans
>
> 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 at lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>>
More information about the dri-devel
mailing list