Re: [PATCH 4/9] drm: Add DRM support for tiny LCD displays

[Noralf TrÃnnes]

Den 23.01.2017 10.28, skrev Daniel Vetter:
> On Sun, Jan 22, 2017 at 07:11:12PM +0100, Noralf Trønnes wrote:
> > tinydrm provides helpers for very simple displays that can use
> > CMA backed framebuffers and need flushing on changes.
> >
> > Signed-off-by: Noralf Trønnes <noralf@xxxxxxxxxxx>
> Looks all pretty. A bunch of ideas below, but all optional. For merging I
> think simplest to first get the core patches in through drm-misc, and then
> you can submit a pull request to Dave for tinydrm+backends (just needs an
> ack for the dt parts from dt maintainers), including MAINTAINERS entry.
> Ack from my side.

So when the tinydrm core patches are in:
- tinydrm patches are posted to dri-devel.
- DT patches need ack.
- If there's no comments or resolved, I send a pull request to Dave.

How often and when do I send pull requests?
(I have seen *-next, *-fixes, * for 4.xx)

Do drivers _always_ need a MAINTANERS entry, or is it preferred,
nice to have, not so important, ...


> > +/**
> > + * tinydrm_display_pipe_update - Display pipe update helper
> > + * @pipe: Simple display pipe
> > + * @old_state: Old plane state
> > + *
> > + * This function does a full framebuffer flush if the plane framebuffer
> > + * has changed. It also handles vblank events. Drivers can use this as their
> > + * &drm_simple_display_pipe_funcs->update callback.
> > + */
> > +void tinydrm_display_pipe_update(struct drm_simple_display_pipe *pipe,
> > +				 struct drm_plane_state *old_state)
> > +{
> > +	struct tinydrm_device *tdev = pipe_to_tinydrm(pipe);
> > +	struct drm_framebuffer *fb = pipe->plane.state->fb;
> > +	struct drm_crtc *crtc = &tdev->pipe.crtc;
> > +
> > +	if (!fb)
> > +		DRM_DEBUG_KMS("fb unset\n");
> > +	else if (!old_state->fb)
> > +		DRM_DEBUG_KMS("fb set\n");
> > +	else if (fb != old_state->fb)
> > +		DRM_DEBUG_KMS("fb changed\n");
> > +	else
> > +		DRM_DEBUG_KMS("No fb change\n");
> > +
> > +	if (fb && (fb != old_state->fb)) {
> > +		pipe->plane.fb = fb;
> > +		if (fb->funcs->dirty)
> > +			fb->funcs->dirty(fb, NULL, 0, 0, NULL, 0);
> I like the idea, but my cunning long-term plan is that we'd extend the
> atomic support to support a dirty rectangle. Together with the
> non-blocking stuff we could then implement fb->funcs->dirty in terms of
> atomic. But here you implement atomic in terms of ->dirty, and we'd end up
> with a loop.
>
> Personally I'd just drop this helper here and move this part into the
> backend modules ...
>
> > +	}
> > +
> > +	if (crtc->state->event) {
> > +		DRM_DEBUG_KMS("crtc event\n");
> > +		spin_lock_irq(&crtc->dev->event_lock);
> > +		drm_crtc_send_vblank_event(crtc, crtc->state->event);
> > +		spin_unlock_irq(&crtc->dev->event_lock);
> > +		crtc->state->event = NULL;
> ... because this here is kinda a hack, since it's not synchronized with
> the screen update. Otoh these tiny panels are kinda special.

Yeah, you're right it's only synchronized if the framebuffer changes.
So this won't catch events that's not page flip events.
afaict DRM_EVENT_VBLANK is the only other event. When is that used?

How about if I check for events as well so the fb is always flushed
if someone wants know?

void tinydrm_display_pipe_update(struct drm_simple_display_pipe *pipe,
                 struct drm_plane_state *old_state)
{
    struct tinydrm_device *tdev = pipe_to_tinydrm(pipe);
    struct drm_framebuffer *fb = pipe->plane.state->fb;
    struct drm_crtc *crtc = &tdev->pipe.crtc;

    if (fb && (fb != old_state->fb || crtc->state->event)) {
        pipe->plane.fb = fb;
        if (fb->funcs->dirty)
            fb->funcs->dirty(fb, NULL, 0, 0, NULL, 0);
    }

    if (crtc->state->event) {
        spin_lock_irq(&crtc->dev->event_lock);
        drm_crtc_send_vblank_event(crtc, crtc->state->event);
        spin_unlock_irq(&crtc->dev->event_lock);
        crtc->state->event = NULL;
    }
}


Or maybe I should send the event in the dirty() function instead?

void tinydrm_display_pipe_update(struct drm_simple_display_pipe *pipe,
                 struct drm_plane_state *old_state)
{
    struct tinydrm_device *tdev = pipe_to_tinydrm(pipe);
    struct drm_framebuffer *fb = pipe->plane.state->fb;
    struct drm_crtc *crtc = &tdev->pipe.crtc;

    if (fb && (fb != old_state->fb || crtc->state->event)) {
        pipe->plane.fb = fb;
        if (fb->funcs->dirty)
            fb->funcs->dirty(fb, NULL, 0, 0, NULL, 0);
    }
}

void tinydrm_send_pending_event(struct tinydrm_device *tdev)
{
    struct drm_crtc *crtc = &tdev->pipe.crtc;

    if (!crtc->state->event)
        return;

    spin_lock_irq(&crtc->dev->event_lock);
    drm_crtc_send_vblank_event(crtc, crtc->state->event);
    spin_unlock_irq(&crtc->dev->event_lock);
    crtc->state->event = NULL;
}

static int mipi_dbi_fb_dirty(struct drm_framebuffer *fb,
                 struct drm_file *file_priv,
                 unsigned int flags, unsigned int color,
                 struct drm_clip_rect *clips,
                 unsigned int num_clips)
{
    <snip>
    mutex_lock(&tdev->dirty_lock);

    <flush>

    tinydrm_send_pending_event(tdev);

    mutex_unlock(&tdev->dirty_lock);

    return ret;
}


Noralf.