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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




Den 24.01.2019 15.53, skrev Hans de Goede:
> 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@xxxxxxxxxxx>
>>>> ---
>>>>   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. 

Indeed that's what I'm doing.

>                               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?
> 

These tiny SPI displays are heavily used in the Maker community which is
my main user base. Most are not professionals and fbdev is easy to use,
so I want to avoid crippling fbdev.

> 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.
> 

If we instead apply the rotation after the framebuffer is created, is it
enough to check fb->modifier == DRM_FORMAT_MOD_LINEAR to see if it's ok
to rotate?

My next task after this patchset is to try and move the modesetting code
from drm_fb_helper to drm_client. I see now that I have to take this
rotation into account as well for the future bootsplash client to show
correctly.

Noralf.

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




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux