| Message ID | 20190120114318.49199-4-noralf@tronnes.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/tinydrm: Remove tinydrm_device | expand |
Hi Noralf. 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 Maybe extend the documentation to list valid values? Have you considered to use the DRM_MODE_ROTATE_XXX constants here? Current tinydrm do not use these and they may not be applicable... Sam
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
[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 >
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@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@lists.freedesktop.org >>> https://lists.freedesktop.org/mailman/listinfo/dri-devel >>
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@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. 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@lists.freedesktop.org >>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel >>>
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 + * + * This function creates a &drm_connector that has one fixed &drm_display_mode + * which will be rotated according to @rotation. + * + * 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 */
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(+)