On 15.09.2016 13:37, Pekka Paalanen wrote:
> On Wed, 14 Sep 2016 11:50:34 +0200
> Armin Krezović <krezovic.ar...@gmail.com> wrote:
>
>> On 13.09.2016 12:49, Pekka Paalanen wrote:
>>> On Thu, 18 Aug 2016 18:42:32 +0200
>>> Armin Krezović <krezovic.ar...@gmail.com> wrote:
>>>
>>>> This is a complete port of the DRM backend that uses
>>>> recently added output handling API for output
>>>> configuration.
>>>>
>>>> Output can be configured at runtime by passing the
>>>> necessary configuration parameters, which can be
>>>> filled in manually or obtained from the configuration
>>>> file using previously added functionality. It is
>>>> required that the scale and transform values are set
>>>> using the previously added functionality.
>>>>
>>>> After everything has been set, output needs to be
>>>> enabled manually using weston_output_enable().
>>>>
>>>> v2:
>>>>
>>>> - Added missing drmModeFreeCrtc() to drm_output_enable()
>>>> cleanup list in case of failure.
>>>> - Split drm_backend_disable() into drm_backend_deinit()
>>>> to accomodate for changes in the first patch in the
>>>> series. Moved restoring original crtc to
>>>> drm_output_destroy().
>>>>
>>>> v3:
>>>>
>>>> - Moved origcrtc allocation to drm_output_set_mode().
>>>> - Swapped connector_get_current_mode() and
>>>> drm_output_add_mode() calls in drm_output_set_mode()
>>>> to match current weston.
>>>> - Moved crtc_allocator and connector_allocator update
>>>> from drm_output_enable() to create_output_for_connector()
>>>> to avoid problems when more than one monitor is connected
>>>> at startup and crtc allocator wasn't updated before
>>>> create_output_for_connector() was called second time,
>>>> resulting in one screen being turned off.
>>>> - Moved crtc_allocator and connector_allocator update from
>>>> drm_output_deinit() to drm_output_destroy(), as it
>>>> should not be called on drm_output_disable().
>>>> - Use weston_compositor_add_pending_output().
>>>> - Bump weston_drm_backend_config version to 2.
>>>>
>>>> Signed-off-by: Armin Krezović <krezovic.ar...@gmail.com>
>>>> ---
>>>> compositor/main.c | 104 ++++++-----
>>>> libweston/compositor-drm.c | 434
>>>> ++++++++++++++++++++++++++-------------------
>>>> libweston/compositor-drm.h | 50 +++---
>>>> 3 files changed, 343 insertions(+), 245 deletions(-)
>>>
>>> Hi Armin,
>>>
>>> all in all, this patch looks very good. There are a few minor bugs to
>>> be squashed, and I did list some future development ideas you are free
>>> to ignore.
>>>
>>> Please wait for reviews for the whole series before re-sending.
>>>
>>> Details inlined below.
>
>
>>>> +static int
>>>> +drm_output_enable(struct weston_output *base)
>>>> +{
>>>> + struct drm_output *output = to_drm_output(base);
>>>> + struct drm_backend *b = to_drm_backend(base->compositor);
>>>> + struct weston_mode *m;
>>>> +
>>>> + output->dpms_prop = drm_get_prop(b->drm.fd, output->connector, "DPMS");
>>>>
>>>> if (b->use_pixman) {
>>>> if (drm_output_init_pixman(output, b) < 0) {
>>>> weston_log("Failed to init output pixman state\n");
>>>> - goto err_output;
>>>> + goto err_free;
>>>> }
>>>> } else if (drm_output_init_egl(output, b) < 0) {
>>>> weston_log("Failed to init output gl state\n");
>>>> - goto err_output;
>>>> + goto err_free;
>>>> }
>>>>
>>>> - output->backlight = backlight_init(drm_device,
>>>> - connector->connector_type);
>>>> if (output->backlight) {
>>>
>>> Any reason you left the backlight_init() call in
>>> create_output_for_connector()? I think it would be more logical here,
>>> being called only when enabled. It shouldn't matter in practise though.
>>>
>>
>> It would, yes. But backlight_init uses struct udev_device, which is passed
>> to create_output_for_connector, and not preserved anywhere.
>
> Hi,
>
> Ooh, ok, that is reason enough.
>
>> Sure, I could preserve that one too, but I didn't like the idea (it's enough
>> we have to preserve the connector).
>>
>> If you still want it to be initialized here, I can preserve struct
>> udev_device
>> just fine.
>>
>>>> weston_log("Initialized backlight, device %s\n",
>>>> output->backlight->path);
>>>> @@ -2442,15 +2385,8 @@ create_output_for_connector(struct drm_backend *b,
>>>> weston_log("Failed to initialize backlight\n");
>>>> }
>
>
>>>> @@ -2578,7 +2644,6 @@ create_outputs(struct drm_backend *b, uint32_t
>>>> option_connector,
>>>> drmModeConnector *connector;
>>>> drmModeRes *resources;
>>>> int i;
>>>> - int x = 0, y = 0;
>>>>
>>>> resources = drmModeGetResources(b->drm.fd);
>>>> if (!resources) {
>>>> @@ -2610,21 +2675,15 @@ create_outputs(struct drm_backend *b, uint32_t
>>>> option_connector,
>>>> (option_connector == 0 ||
>>>> connector->connector_id == option_connector)) {
>>>> if (create_output_for_connector(b, resources,
>>>> - connector, x, y,
>>>> - drm_device) < 0) {
>>>> + connector, drm_device)
>>>> < 0) {
>>>> drmModeFreeConnector(connector);
>>
>> ---^
>>
>>>> continue;
>>>> }
>>>> -
>>>> - x += container_of(b->compositor->output_list.prev,
>>>> - struct weston_output,
>>>> - link)->width;
>>>> }
>>>> -
>>>> - drmModeFreeConnector(connector);
>>>
>>> The Free should be moved into an else-branch instead of removed,
>>> shouldn't it?
>>>
>>>
>>
>> There's already one above (when if fails). This one would run after
>> create_output_for_connector has been ran (since previously the
>> connector wasn't preserved).
>
> I meant for the case when connector->connection == DRM_MODE_CONNECTED
> is not true.
>
>
>>>> }
>>>>
>>>> - if (wl_list_empty(&b->compositor->output_list))
>>>> + if (wl_list_empty(&b->compositor->output_list) &&
>>>> + wl_list_empty(&b->compositor->pending_output_list))
>>>> weston_log("No currently active connector found.\n");
>>>>
>>>> drmModeFreeResources(resources);
>>>> @@ -2638,7 +2697,6 @@ update_outputs(struct drm_backend *b, struct
>>>> udev_device *drm_device)
>>>> drmModeConnector *connector;
>>>> drmModeRes *resources;
>>>> struct drm_output *output, *next;
>>>> - int x = 0, y = 0;
>>>> uint32_t connected = 0, disconnects = 0;
>>>> int i;
>>>>
>>>> @@ -2664,23 +2722,11 @@ update_outputs(struct drm_backend *b, struct
>>>> udev_device *drm_device)
>>>> connected |= (1 << connector_id);
>>>>
>>>> if (!(b->connector_allocator & (1 << connector_id))) {
>>>> - struct weston_output *last =
>>>> - container_of(b->compositor->output_list.prev,
>>>> - struct weston_output, link);
>>>> -
>>>> - /* XXX: not yet needed, we die with 0 outputs */
>>>> - if (!wl_list_empty(&b->compositor->output_list))
>>>> - x = last->x + last->width;
>>>> - else
>>>> - x = 0;
>>>> - y = 0;
>>>> create_output_for_connector(b, resources,
>>>> - connector, x, y,
>>>> - drm_device);
>>>> + connector, drm_device);
>>>> weston_log("connector %d connected\n", connector_id);
>>>>
>>>> }
>>>> - drmModeFreeConnector(connector);
>>>
>>> drmModeFreeConnector() should stay but in a new else-branch, right?
>
> This is a similar case, I'm referring to the else branch of
> condition !(b->connector_allocator & (1 << connector_id)).
>
>
>>>
>>>
>>>> }
>>>> drmModeFreeResources(resources);
>>>>
>>>> @@ -2695,6 +2741,16 @@ update_outputs(struct drm_backend *b, struct
>>>> udev_device *drm_device)
>>>> drm_output_destroy(&output->base);
>>>> }
>>>> }
>>>> +
>>>> + wl_list_for_each_safe(output, next,
>>>> &b->compositor->pending_output_list,
>>>> + base.link) {
>>>> + if (disconnects & (1 << output->connector_id)) {
>>>> + disconnects &= ~(1 << output->connector_id);
>>>> + weston_log("connector %d disconnected\n",
>>>> + output->connector_id);
>>>> + drm_output_destroy(&output->base);
>>>> + }
>>>> + }
>>>> }
>>>> }
>>>>
>
>
> Thanks,
> pq
> Ah, right. Now it makes sense. Will fix it.
signature.asc
Description: OpenPGP digital signature
_______________________________________________ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
