[PATCH] drm: Add Grain Media GM12U320 driver

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

Interesting, good luck with that.

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

Regards,

Hans