Hi

Am 12.11.19 um 16:13 schrieb Daniel Vetter:

On Tue, Nov 12, 2019 at 3:51 PM Thomas Zimmermann [off-list ref] wrote:

quoted

Hi

Am 12.11.19 um 15:31 schrieb Daniel Vetter:

quoted

On Tue, Nov 12, 2019 at 3:03 PM Thomas Zimmermann [off-list ref] wrote:

quoted

Hi

Am 12.11.19 um 14:40 schrieb Daniel Vetter:

quoted

On Tue, Nov 12, 2019 at 12:55 PM Thomas Zimmermann [off-list ref] wrote:

quoted

Hi

Am 08.11.19 um 16:37 schrieb Noralf Trønnes:

quoted

Den 08.11.2019 13.33, skrev Thomas Zimmermann:

quoted

The udl driver can use the generic fbdev implementation. Convert it.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---

quoted

diff --git a/drivers/gpu/drm/udl/udl_drv.c b/drivers/gpu/drm/udl/udl_drv.c
index 563cc5809e56..55c0f9dfee29 100644
--- a/drivers/gpu/drm/udl/udl_drv.c
+++ b/drivers/gpu/drm/udl/udl_drv.c

quoted

@@ -47,6 +48,8 @@ static struct drm_driver driver = {
     .driver_features = DRIVER_MODESET | DRIVER_GEM,
     .release = udl_driver_release,

+    .lastclose = drm_fb_helper_lastclose,
+

No need to set this, it's already wired up:

drm_lastclose -> drm_client_dev_restore -> drm_fbdev_client_restore ->
drm_fb_helper_lastclose

quoted

     /* gem hooks */
     .gem_create_object = udl_driver_gem_create_object,

quoted

diff --git a/drivers/gpu/drm/udl/udl_fb.c b/drivers/gpu/drm/udl/udl_fb.c
index f8153b726343..afe74f892a2b 100644
--- a/drivers/gpu/drm/udl/udl_fb.c
+++ b/drivers/gpu/drm/udl/udl_fb.c

@@ -20,19 +20,9 @@

 #include "udl_drv.h"

-#define DL_DEFIO_WRITE_DELAY    (HZ/20) /* fb_deferred_io.delay in jiffies */
-
-static int fb_defio = 0;  /* Optionally enable experimental fb_defio mmap support */
 static int fb_bpp = 16;

 module_param(fb_bpp, int, S_IWUSR | S_IRUSR | S_IWGRP | S_IRGRP);

Maybe fb_bpp can be dropped too?

Sure, makes sense.

The driver exposes a preferred color depth of 24 bpp, which we may want
to change to 16 then. The internal framebuffer is only 16 bpp anyway.

Just something that crossed my mind: Should we ensure that the
preferred format of the primary plane (should be the first in the
format array) matches up with the preferred bpp setting? Maybe even
enforce that for drivers with an explicit primary plane (i.e. atomic
drivers). I think tiny drivers get this right already.

IMHO that makes if the userspace can handle it. The preferred bpp could
also be retrieved from the formats array automatically. What about HW
with multiple CRTCs with different format defaults (sounds weird, I know)?

Ime I haven't seen such a case yet. What I have seen is that the most
preferred format might be some fancy compressed format, which not all
formats support. But which you can't render into without mesa anyway,
so really doesn't matter for preferred bpp.

quoted

WRT udl: For v3 of this patchset I've set the preferred color depth to
32 bpp; although the internal FB is always at 16 bpp. Because when I
tested with a dual-screen setup (radeon + udl) X11 didn't support the 16
bpp output on the second screen (the one driven by udl). Only setting
both screen to 32 bpp worked out of the box. And the preferred 24 bpp
are not even supported by udl.

Uh, if we can only set preferred bpp to make X happy, and X can only
support one preferred bpp, then everyone needs to set 32bit. Which
defeats the point (and we'd need to hardcode it to 32bpp). Is this
really the case?

I guess it would have worked if both screens preferred 16 bpp.

Just noticed that current depth is 24 bpp ... does that work with
current X? If not I think we should actually set it to 16 bpp, and fix
up X. Not as in "make it handle multi-bpp", that's too hard, but make
it pick a common format that works for all drivers (which usually
means go with 32bpp). Since if we just go with 32bpp to paper over X,
then the preferred bpp uapi becomes meaningless.

The good news is that it apparently works with planes correctly configured.

I tested with udl being converted to simple pipe and universal planes.
It exposes RGB565, XRGB8888 and preferred_depth=16. X did the correct
thing and choose a format the works with radeon and udl.

For the next iteration of the fbdev conversion, I'll just keep the
current values. And once converted to universal planes, we can set the
optimal values instead.

Best regards
Thomas

-Daniel

quoted

Best regards
Thomas

quoted

-Daniel

quoted

Best regards
Thomas

quoted

-Daniel

quoted

Best regards
Thomas

quoted

It's possible to set it on the command line:

video=<xres>x<yres>-<bpp>

I haven't tried it so I can't say for certain that it actually works>
Ref: Documentation/fb/modedb.rst and drm_fb_helper_single_fb_probe()

quoted

-module_param(fb_defio, int, S_IWUSR | S_IRUSR | S_IWGRP | S_IRGRP);
-

quoted

diff --git a/drivers/gpu/drm/udl/udl_modeset.c b/drivers/gpu/drm/udl/udl_modeset.c
index bc1ab6060dc6..1517d5e881b8 100644
--- a/drivers/gpu/drm/udl/udl_modeset.c
+++ b/drivers/gpu/drm/udl/udl_modeset.c

quoted

@@ -422,7 +423,7 @@ static int udl_crtc_init(struct drm_device *dev)

 static const struct drm_mode_config_funcs udl_mode_funcs = {
     .fb_create = udl_fb_user_fb_create,
-    .output_poll_changed = NULL,
+    .output_poll_changed = drm_fb_helper_output_poll_changed,

No need to set this, it's already wired up:

drm_kms_helper_hotplug_event -> drm_client_dev_hotplug ->
drm_fbdev_client_hotplug -> drm_fb_helper_hotplug_event

Noralf.

quoted

 };

 int udl_modeset_init(struct drm_device *dev)

--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer

--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer

--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer