[PATCH] drm: Add Grain Media GM12U320 driver

[Noralf Trønnes]

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.

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