[PATCH] drm: Add Grain Media GM12U320 driver
Hans de Goede
hdegoede at redhat.com
Wed Jul 17 20:37:23 UTC 2019
Hi Noralf,
Thank you for the review.
On 17-07-19 17:24, Noralf Trønnes wrote:
>
>
> Den 12.07.2019 20.53, skrev Hans de Goede:
>> Add a modesetting driver for Grain Media GM12U320 based devices
>> (primarily Acer C120 projector, but there may be compatible devices).
>>
>> This is based on the fb driver from Viacheslav Nurmekhamitov:
>> https://github.com/slavrn/gm12u320
>>
>> This driver uses drm_simple_display_pipe to deal with all the atomic
>> stuff, gem_shmem_helper functions for buffer management and
>> drm_fbdev_generic_setup for fbdev emulation, so that leaves the driver
>> itself with only the actual code for talking to the gm13u320 chip,
>> leading to a nice simple and clean driver.
>
> Yeah, it's a lot smaller now than when it was first submitted a couple
> of years ago ;-)
Ack, this is much better now.
>> Signed-off-by: Hans de Goede <hdegoede at redhat.com>
>> ---
>> MAINTAINERS | 5 +
>> drivers/gpu/drm/Kconfig | 2 +
>> drivers/gpu/drm/Makefile | 1 +
>> drivers/gpu/drm/gm12u320/Kconfig | 9 +
>> drivers/gpu/drm/gm12u320/Makefile | 2 +
>> drivers/gpu/drm/gm12u320/gm12u320.c | 817 ++++++++++++++++++++++++++++
>> 6 files changed, 836 insertions(+)
>> create mode 100644 drivers/gpu/drm/gm12u320/Kconfig
>> create mode 100644 drivers/gpu/drm/gm12u320/Makefile
>> create mode 100644 drivers/gpu/drm/gm12u320/gm12u320.c
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index e2db0d467966..754d884eb26b 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -5025,6 +5025,11 @@ S: Maintained
>> F: drivers/gpu/drm/panel/panel-feiyang-fy07024di26a30d.c
>> F: Documentation/devicetree/bindings/display/panel/feiyang,fy07024di26a30d.txt
>>
>> +DRM DRIVER FOR GRAIN MEDIA GM12U320 PROJECTORS
>> +M: Hans de Goede <hdegoede at redhat.com>
>
> I assume this will be a drm-misc driver:
>
> T: git git://anongit.freedesktop.org/drm/drm-misc
Ack, will fix for v2.
>> +S: Maintained
>> +F: drivers/gpu/drm/gm12u320/
>> +
>> DRM DRIVER FOR ILITEK ILI9225 PANELS
>> M: David Lechner <david at lechnology.com>
>> S: Maintained
<snip>
>> diff --git a/drivers/gpu/drm/gm12u320/gm12u320.c b/drivers/gpu/drm/gm12u320/gm12u320.c
>> new file mode 100644
>> index 000000000000..ccbbd62f444d
>> --- /dev/null
>> +++ b/drivers/gpu/drm/gm12u320/gm12u320.c
<snip>
>> +static void gm12u320_pipe_update(struct drm_simple_display_pipe *pipe,
>> + struct drm_plane_state *old_state)
>> +{
>> + struct drm_plane_state *state = pipe->plane.state;
>> + struct drm_crtc *crtc = &pipe->crtc;
>> + struct drm_rect rect;
>> +
>> + if (drm_atomic_helper_damage_merged(old_state, state, &rect))
>> + gm12u320_fb_mark_dirty(pipe->plane.state->fb, &rect);
>
> I'm about to write a usb display driver myself, so I'm curious about why
> you punt off the update to a worker instead of doing the update inline?
There are 2 reasons:
1) Doing the update inline is going to take a while, where as userspace
desktop code expects the flip to be nearly instant, so if we block long
here we are introducing significant latency to various userspace code
paths which is undesirable.
2) The hardware in question falls back to showing a builtin screen
with driver installation instruction if you do not send it a new
frame every 2 seconds. So if a desktop environment is smart (energy
consumption aware) enough to not re-render needlessly and the user
is just sitting there watching at the screen (so the ui is idle),
then without the worker we will get this driver install screen
after 2 seconds instead of the desktop. This is also why the loop
in the worker uses wait_event_timeout() instead of plain wait_event()
Interesting that you are working on an usb display driver, can
you share for which hardware this is?
A couple of years ago (around the time I posted the first version of
a driver for the gm12u320) I started this side project to add support
for some video over USB devices. I have hardware for; and eventually
hope to add kms support for the following:
-Fresco Logic FL2000 chip out of tree driver here:
https://github.com/FrescoLogic/FL2000
This one is used in manycheap USB3 (A connector) to VGA dongles
-xf86-video-sisusb supported devices
-Philips PicoPix PPX2055 libam7xxx / am7xxx based devices:
https://git.ao2.it/libam7xxx.git/
If you happen to be working on support for one of those, please let
me know so I can avoid doing double work. I hope that now that I've
found some time to properly tackle the gm12u320 case I'll also find
time for some of these other soonish. The simple pipe stuff is really
nice and makes working on this a lot easier, so I'm certainly motivated
to spend some of my spare time on this now :)
>> +
>> + if (crtc->state->event) {
>> + spin_lock_irq(&crtc->dev->event_lock);
>> + drm_crtc_send_vblank_event(crtc, crtc->state->event);
>> + crtc->state->event = NULL;
>> + spin_unlock_irq(&crtc->dev->event_lock);
>> + }
>> +}
>> +
>> +static const struct drm_simple_display_pipe_funcs gm12u320_pipe_funcs = {
>> + .enable = gm12u320_pipe_enable,
>> + .disable = gm12u320_pipe_disable,
>> + .update = gm12u320_pipe_update,
>> +};
>> +
>> +static const uint32_t gm12u320_pipe_formats[] = {
>> + DRM_FORMAT_XRGB8888,
>> +};
>> +
>> +static const uint64_t gm12u320_pipe_modifiers[] = {
>> + DRM_FORMAT_MOD_LINEAR,
>> + DRM_FORMAT_MOD_INVALID
>> +};
>> +
>> +static void gm12u320_driver_release(struct drm_device *dev)
>> +{
>> + struct gm12u320_device *gm12u320 = dev->dev_private;
>> +
>> + gm12u320_usb_free(gm12u320);
>> + drm_mode_config_cleanup(dev);
>> + drm_dev_fini(dev);
>> + kfree(gm12u320);
>> +}
>> +
>> +DEFINE_DRM_GEM_SHMEM_FOPS(gm12u320_fops);
>> +
>> +static struct drm_driver gm12u320_drm_driver = {
>> + .driver_features = DRIVER_MODESET | DRIVER_GEM | DRIVER_ATOMIC |
>> + DRIVER_PRIME,
>> +
>> + .name = DRIVER_NAME,
>> + .desc = DRIVER_DESC,
>> + .date = DRIVER_DATE,
>> + .major = DRIVER_MAJOR,
>> + .minor = DRIVER_MINOR,
>> +
>> + .release = gm12u320_driver_release,
>> + .fops = &gm12u320_fops,
>> + DRM_GEM_SHMEM_DRIVER_OPS,
>> +};
>> +
>> +static const struct drm_mode_config_funcs gm12u320_mode_config_funcs = {
>> + .fb_create = drm_gem_fb_create_with_dirty,
>> + .atomic_check = drm_atomic_helper_check,
>> + .atomic_commit = drm_atomic_helper_commit,
>> +};
>> +
>> +static int gm12u320_usb_probe(struct usb_interface *interface,
>> + const struct usb_device_id *id)
>> +{
>> + struct gm12u320_device *gm12u320;
>> + struct drm_device *dev;
>> + int ret;
>> +
>> + /*
>> + * The gm12u320 presents itself to the system as 2 usb mass-storage
>> + * interfaces, we only care about / need the first one.
>> + */
>> + if (interface->cur_altsetting->desc.bInterfaceNumber != 0)
>> + return -ENODEV;
>> +
>> + gm12u320 = kzalloc(sizeof(*gm12u320), GFP_KERNEL);
>> + if (gm12u320 == NULL)
>> + return -ENOMEM;
>> +
>> + gm12u320->udev = interface_to_usbdev(interface);
>> + INIT_WORK(&gm12u320->fb_update.work, gm12u320_fb_update_work);
>> + mutex_init(&gm12u320->fb_update.lock);
>> + init_waitqueue_head(&gm12u320->fb_update.waitq);
>> +
>> + dev = &gm12u320->dev;
>> + ret = drm_dev_init(dev, &gm12u320_drm_driver, &interface->dev);
>> + if (ret) {
>> + kfree(gm12u320);
>> + return ret;
>> + }
>> + dev->dev_private = gm12u320;
>> +
>> + drm_mode_config_init(dev);
>> + dev->mode_config.min_width = GM12U320_USER_WIDTH;
>> + dev->mode_config.max_width = GM12U320_USER_WIDTH;
>> + dev->mode_config.min_height = GM12U320_HEIGHT;
>> + dev->mode_config.max_height = GM12U320_HEIGHT;
>> + dev->mode_config.preferred_depth = 24;
>
> Why do you need to set this when the driver supports only one format?
I copy and pasted this from the drivers/gpu/drm/cirrus/cirrus.c code
and never gave it a second thought. I will remove it for v2 of the patch.
>
>> + dev->mode_config.prefer_shadow = 0;
>
> No need to set this, it's already zero.
Ack, will fix.
>> + dev->mode_config.funcs = &gm12u320_mode_config_funcs;
>> +
>> + ret = gm12u320_usb_alloc(gm12u320);
>> + if (ret)
>> + goto err_put;
>> +
>> + ret = gm12u320_set_ecomode(gm12u320);
>> + if (ret)
>> + goto err_put;
>> +
>> + ret = gm12u320_conn_init(gm12u320);
>> + if (ret)
>> + goto err_put;
>> +
>> + ret = drm_simple_display_pipe_init(&gm12u320->dev,
>> + &gm12u320->pipe,
>> + &gm12u320_pipe_funcs,
>> + gm12u320_pipe_formats,
>> + ARRAY_SIZE(gm12u320_pipe_formats),
>> + gm12u320_pipe_modifiers,
>> + &gm12u320->conn);
>> + if (ret)
>> + goto err_put;
>> +
>> + drm_mode_config_reset(dev);
>> +
>> + usb_set_intfdata(interface, dev);
>> + ret = drm_dev_register(dev, 0);
>> + if (ret)
>> + goto err_put;
>> +
>> + drm_fbdev_generic_setup(dev, dev->mode_config.preferred_depth);
>
> I can't see how this can give you working fbdev.
>
> We enter here with preferred_bpp=24:
>
> static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper,
> int preferred_bpp)
> {
> ...
> sizes.surface_depth = 24;
> sizes.surface_bpp = 32;
> ...
> if (preferred_bpp != sizes.surface_bpp)
> sizes.surface_depth = sizes.surface_bpp = preferred_bpp;
>
> We enter here with sizes.surface_depth=24 and sizes.surface_bpp=24:
>
> int drm_fb_helper_generic_probe(struct drm_fb_helper *fb_helper,
> struct drm_fb_helper_surface_size *sizes)
> {
> ...
> format = drm_mode_legacy_fb_format(sizes->surface_bpp,
> sizes->surface_depth);
>
> We enter here with bpp=24 and depth=24:
>
> uint32_t drm_mode_legacy_fb_format(uint32_t bpp, uint32_t depth)
> {
> ...
> switch (bpp) {
> ...
> case 24:
> if (depth == 24)
> fmt = DRM_FORMAT_RGB888;
> break;
>
> And the driver doesn't support this format...?
I must admit I spend a lot of time testing the driver as
PRIME secondary GPU video output, filing a bunch of xserver
bugs I hit:
"Xorg's software-cursor support + mutter results in a mess"
https://gitlab.freedesktop.org/xorg/xserver/issues/828
"Software cursor results in pointer trails"
https://gitlab.freedesktop.org/xorg/xserver/issues/829
"xserver does not see secondary GPU devices when they are present when Xorg starts"
https://gitlab.freedesktop.org/xorg/xserver/issues/830
"Xorg crashes when unplugging a USB attached secondary (output only) GPU"
https://gitlab.freedesktop.org/xorg/xserver/issues/831
I've got a set of fixes for 828 for the 1.20 branch, the
other 3 happen only on master. I've not submitted the 828
fixes upstream yet, since the fixes have issues with master...
In case you need a bug free 1.20 branch for testing the usb driver
you are working on, you can find my work on this here:
https://gitlab.freedesktop.org/jwrdegoede/xserver/commits/server-1.20-branch
For 828 you specifically want the 3 modesetting commits and
"autobind GPUs to the screen" also makes live a lot easier and
you probably also want "Use intel ddx only on pre-gen4 hw, newer ones will fall back to modesetting"
at least in my testing the Intel DDX and PRIME had some issues.
Anyways this was sorta a rabbithole I got sucked into and I
completely forgot to test the fbdev emulation (my bad). I will give
the fbdev emulation a test run (with the preferred_bpp=24 removed)
before posting v2.
Regards,
Hans
More information about the dri-devel
mailing list