[PATCH] drm: Add Grain Media GM12U320 driver

[Noralf Trønnes]

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 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(+)
>>
>> <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.