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

[Thomas Zimmermann]

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.

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

-------------- 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/20191112/2a4aa966/attachment.sig>