Re: [Intel-gfx] [PATCH 2/2] drm/tinydrm: Make fb_dirty into a lower level hook

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Fri, Mar 23, 2018 at 03:58:06PM +0200, Ville Syrjälä wrote:
> On Fri, Mar 23, 2018 at 02:37:23PM +0100, Noralf Trønnes wrote:
> > 
> > Den 23.03.2018 12.31, skrev Ville Syrjälä:
> > > On Fri, Mar 23, 2018 at 12:43:58AM +0100, Noralf Trønnes wrote:
> > >>
> > >> Den 22.03.2018 21.27, skrev Ville Syrjala:
> > >>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > >>>
> > >>> mipi_dbi_enable_flush() wants to call the fb->dirty() hook from the
> > >>> bowels of the .atomic_enable() hook. That prevents us from taking the
> > >>> plane mutex in fb->dirty() unless we also plumb down the acquire
> > >>> context.
> > >>>
> > >>> Instead it seems simpler to split the fb->dirty() into a tinydrm
> > >>> specific lower level hook that can be called from
> > >>> mipi_dbi_enable_flush() and from a generic higher level
> > >>> tinydrm_fb_dirty() helper. As we don't have a tinydrm specific
> > >>> vfuncs table we'll just stick it into tinydrm_device directly
> > >>> for now.
> > >> The long term goal is to try and get rid of tinydrm.ko by moving stuff
> > >> elsewhere as it's kind of a middle layer. So I'd really like to avoid
> > >> adding a callback like this.
> > >> Hopefully we can work out a solution based on my suggestion in the
> > >> 'drm: Eliminate plane->fb/crtc usage for atomic drivers' thread.
> > > I don't want to start redesigning the entire architecture at
> > > this point. That would also cause a bigger risk of regression and
> > > then we'd potentially have to try and revert the entire plane->fb
> > > thing, which would not be fun if any significant changes have
> > > occurred in the meantime.
> > >
> > >> If we do need a hook, I prefer that we add it to
> > >> drm_simple_display_pipe_funcs.
> > > If you have plans to redesign tinydrm anyway it seems to me that
> > > a temporary hook in tinydrm is may be a bit less intrusive than
> > > inflicting it on the simple_kms_helper.
> > 
> > Yes you're right of course, what was I thinking.
> > 
> > You missed out on one call site:
> > 
> > diff --git a/drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c 
> > b/drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c
> > index 11ae950b0fc9..7924eb59c2e1 100644
> > --- a/drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c
> > +++ b/drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c
> > @@ -124,11 +124,8 @@ void tinydrm_display_pipe_update(struct 
> > drm_simple_display_pipe *pipe,
> >          struct drm_framebuffer *fb = pipe->plane.state->fb;
> >          struct drm_crtc *crtc = &tdev->pipe.crtc;
> > 
> > -       if (fb && (fb != old_state->fb)) {
> > -               pipe->plane.fb = fb;
> > -               if (fb->funcs->dirty)
> > -                       fb->funcs->dirty(fb, NULL, 0, 0, NULL, 0);
> > -       }
> > +       if (fb && (fb != old_state->fb))
> > +               tdev->fb_dirty(fb, NULL, 0, 0, NULL, 0);
> > 
> >          if (crtc->state->event) {
> >                  spin_lock_irq(&crtc->dev->event_lock);
> > 
> > With that fixed, series is:
> > 
> > Reviewed-by: Noralf Trønnes <noralf@tronnes.org>
> 
> Awesome. Thanks. And thanks for catching that extra dirty() call. I'll
> go and fix it up.

OK, I posted the fixed version.

Would you be interested in running some kind of smoke test on this
before I push it? I'd hate to break things for you, and unfortunately
(or maybe fortunately :) I don't have any hardware to test this.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel