On Wed, Nov 13, 2019 at 10:51:51AM +0100, Thomas Zimmermann wrote:
> Hi
>
> Am 12.11.19 um 16:13 schrieb Daniel Vetter:
> > On Tue, Nov 12, 2019 at 3:51 PM Thomas Zimmermann <tzimmermann@xxxxxxx> wrote:
> >>
> >> Hi
> >>
> >> Am 12.11.19 um 15:31 schrieb Daniel Vetter:
> >>> On Tue, Nov 12, 2019 at 3:03 PM Thomas Zimmermann <tzimmermann@xxxxxxx> wrote:
> >>>>
> >>>> Hi
> >>>>
> >>>> Am 12.11.19 um 14:40 schrieb Daniel Vetter:
> >>>>> On Tue, Nov 12, 2019 at 12:55 PM Thomas Zimmermann <tzimmermann@xxxxxxx> 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@xxxxxxx>
> >>>>>>>> ---
> >>>>>>>
> >>>>>>>> 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.
Awesome!
> 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.
Yeah sounds good. Worst case it should at least make it easier to undo the
bpp change.
-Daniel
>
> 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
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
