Den 17.07.2019 22.37, skrev Hans de Goede: > 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@xxxxxxxxxx> >>> --- >>> 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(+) <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? > It's more of a generic usb display solution. This is what I replied to Sam yesterday when discussing tiny SPI displays: When I'm done with the tinydrm cleanup, I'm going to work on an idea I have: turn the Raspberry Pi Zero into a $5 USB to HDMI/SDTV/DSI/DPI/SPI-display adapter. I'm planning to write a generic USB host display driver with a matching Linux OTG device driver. I plan to make it easy to do the display OTG side on a microcontroller as well. This way it will be possible for manufacturers to do USB connected displays of (nearly) all sizes without having to write a Linux driver. I'm going to try and do a generic regmap over USB thing that I can put a generic usb display on to of. The idea is that this regmap can be used for generic gpio over usb, adc over usb, etc. I don't know if it will work out in the end but I'll give it a go. >>> + >>> + 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); >>> + } I'm wondering about this signaling here, you're signaling page flip done before the display has been updated. Shouldn't you do that in the worker after the update is sent? <snip> > 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 Thanks for working on this, I wouldn't know where to begin. I've briefly looked at the xserver code, but realised that life is too short to enter into that universe. Noralf. _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
