Hi Hans, Den 20.07.2019 15.44, skrev Hans de Goede: > Hi Noralf, > > On 18-07-19 14:07, Noralf Trønnes wrote: >> >> >> 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() >>> <snip> >>>>> + >>>>> + 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? > > I've considered doing this, but: > > When connected over USB2 the maximum speed with which we can send > updates is about 30 frames per second, but this assumes that the > bus idle, if the bus is in use the time per frame will vary wildly, > sending vblank events to userspace with random (bus usage dependent) > intervals is likely to confuse some userspace code, since IIRC at > least some code tries to predict when it needs to start rendering the > next frame to be able to flip to it just before the vblank (the later > the rendering is started the less the latency). > > Also I'm afraid that with multiple outputs, some compositors might > limit the framerate to the slowest one. > > Another problem is that we do not really know when the flip happens, > looking at the reverse-engineered protocol it seems that the device > has 2 buffers, and we update 1 while the other is being scanned out > and then send a command to flip. The flip will happen after we send > the command, but we do not know when it actually happens. > > Since we do not have accurate vblank events anyway I've chosen to > do what the new simple-pipe based cirrus and the udl drivers are doing, > immediately post the event from the update / crtc_flip callback. > Since this is already done by 2 semi popular drivers I expect > userspace to be able to handle this. > > Note that if update gets called before the previous frame has been > consumed by the worker (which copies its to usb transfer buffers > in one go without waiting for io) then it will be replaced, so > if we are achieving say 30 fps and userspace sends us frames at > 60 fps (synced to another monitor) then we will always draw the > latest frame given to us. > Thanks for the details it's useful for me to know when I start writing a similar type driver. Noralf. _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
