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
