[PATCH 1/4] drm/udl: Replace fbdev code with generic emulation

Wed Nov 13 09:51:51 UTC 2019

Hi

Am 12.11.19 um 16:13 schrieb Daniel Vetter:
> On Tue, Nov 12, 2019 at 3:51 PM Thomas Zimmermann <tzimmermann at suse.de> wrote:
>>
>> Hi
>>
>> Am 12.11.19 um 15:31 schrieb Daniel Vetter:
>>> On Tue, Nov 12, 2019 at 3:03 PM Thomas Zimmermann <tzimmermann at suse.de> wrote:
>>>>
>>>> Hi
>>>>
>>>> Am 12.11.19 um 14:40 schrieb Daniel Vetter:
>>>>> On Tue, Nov 12, 2019 at 12:55 PM Thomas Zimmermann <tzimmermann at suse.de> wrote:
>>>>>>
>>>>>> Hi
>>>>>>
>>>>>> Am 08.11.19 um 16:37 schrieb Noralf Trønnes:
>>>>>>>
>>>>>>>
>>>>>>> Den 08.11.2019 13.33, skrev Thomas Zimmermann:
>>>>>>>> The udl driver can use the generic fbdev implementation. Convert it.
>>>>>>>>
>>>>>>>> Signed-off-by: Thomas Zimmermann <tzimmermann at suse.de>
>>>>>>>> ---
>>>>>>>
>>>>>>>> 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
>>>>>>>
>>>>>>>> @@ -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
>>>>>>>
>>>>>>>>      /* gem hooks */
>>>>>>>>      .gem_create_object = udl_driver_gem_create_object,
>>>>>>>>
>>>>>>>
>>>>>>>> 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.
>>>
>>>> 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
> 
>>
>> Best regards
>> Thomas
>>
>>> -Daniel
>>>
>>>>
>>>> Best regards
>>>> Thomas
>>>>
>>>>> -Daniel
>>>>>
>>>>>>
>>>>>> Best regards
>>>>>> Thomas
>>>>>>
>>>>>>>
>>>>>>> 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()
>>>>>>>
>>>>>>>> -module_param(fb_defio, int, S_IWUSR | S_IRUSR | S_IWGRP | S_IRGRP);
>>>>>>>> -
>>>>>>>
>>>>>>>> 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
>>>>>>>
>>>>>>>> @@ -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.
>>>>>>>
>>>>>>>>  };
>>>>>>>>
>>>>>>>>  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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20191113/257dc5c8/attachment-0001.sig>