Re: [PATCH 3/4] drm: Introduce drm_fb_helper_prepare()

From: Thierry Reding <thierry.reding@gmail.com>
To: Daniel Vetter <daniel@ffwll.ch>
Cc: linux-samsung-soc@vger.kernel.org,
	Daniel Vetter <daniel.vetter@ffwll.ch>,
	dri-devel@lists.freedesktop.org, linux-tegra@vger.kernel.org,
	intel-gfx@lists.freedesktop.org,
	linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH 3/4] drm: Introduce drm_fb_helper_prepare()
Date: Wed, 23 Apr 2014 15:44:48 +0200	[thread overview]
Message-ID: <20140423134447.GB28031@ulmo> (raw)
In-Reply-To: <20140423073548.GW10722@phenom.ffwll.local>

[-- Attachment #1.1: Type: text/plain, Size: 3981 bytes --]

On Wed, Apr 23, 2014 at 09:35:48AM +0200, Daniel Vetter wrote:
> On Wed, Apr 23, 2014 at 09:14:41AM +0200, Thierry Reding wrote:
> > On Tue, Apr 22, 2014 at 05:54:06PM +0200, Daniel Vetter wrote:
> > > On Tue, Apr 22, 2014 at 04:42:20PM +0200, Thierry Reding wrote:
> > [...]
> > > > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> > [...]
> > > > @@ -502,6 +503,33 @@ static void drm_fb_helper_crtc_free(struct drm_fb_helper *helper)
> > > >  }
> > > >  
> > > >  /**
> > > > + * drm_fb_helper_prepare - setup a drm_fb_helper structure
> > > > + * @dev: DRM device
> > > > + * @helper: driver-allocated fbdev helper structure to set up
> > > > + * @funcs: pointer to structure of functions associate with this helper
> > > > + *
> > > > + * Sets up the bare minimum to make the framebuffer helper usable. This is
> > > > + * useful to implement race-free initialization of the polling helpers. In
> > > > + * that case a typical sequence would be:
> > > > + *
> > > > + *   - call drm_fb_helper_prepare()
> > > > + *   - set dev->mode_config.funcs
> > > 
> > > This step is done in drm_fb_helper_prepare already.
> > 
> > drm_fb_helper_prepare() sets fb_helper->funcs. dev->mode_config.funcs
> > needs to be set because it gets called by drm_kms_helper_hotplug_event()
> > which in turn is called by output_poll_execute(), which can be called at
> > any point after drm_kms_helper_poll_init(). It could be scheduled
> > immediately by drm_kms_helper_poll_enable().
> > 
> > I wonder if perhaps we should be wrapping that into a function as well.
> > Currently this is only documented in code by the drivers that call this.
> > But since it's only a single step it doesn't seem worth it. Perhaps if
> > we rolled the min/max_width/height into that function as well it would
> > be more worth it? That could be difficult to do since a couple of
> > drivers need to set those depending on the chipset generation.
> 
> Oh I've misread this step for the fb_helper->funcs assignment. Makes sense
> and I don't think we need to augment your kerneldoc more to explain this.
> 
> > > > + *   - perform driver-specific setup
> 
> Hm, maybe clarify this as "initialize modeset objects like crtcs, encoders
> and connectors"? Since that's the important part we need to get done here.
> 
> > > > + *   - call drm_kms_helper_poll_init()
> > > 
> > > Maybe mention that after this you can start to handle hpd events using the
> > > probe helpers?
> > 
> > Isn't that somewhat implied already? The poll helpers call directly the
> > dev->mode_config.funcs->output_poll_changed() function, which has
> > already been set up earlier.
> 
> I've more meant that after this it's save for drivers to enable hpd
> interrupts and start to process them. I.e.
> 
> 	- enable interrupts and start processing hpd events
> 
> as an additional step to make it really cleear how it all fits together.
> Otherwise driver authors are left wondering wtf this isn't just one
> function call to do it all for them ;-)
> 
> > > > + *   - call drm_fb_helper_init()
> > > > + *   - call drm_fb_helper_single_add_all_connectors()
> > > > + *   - call drm_fb_helper_initial_config()
> > > 
> > > Does this list look sane in the generated DocBook? Afaik kerneldoc
> > > unfortunately lacks any form of markup, at least afaik :(
> > 
> > I must admit I didn't check. I'll make sure I do that before sending out
> > the next version.
> 
> If it looks ugly I'm ok as-is, just wondered. Kerneldoc is a bit
> simplistic ime.

In the second version I just sent out I ended up moving the description
of this sequence into the fbdev helper section rather than keeping it in
the description of the drm_fb_helper_prepare() function, since the new
function is really just a part of the whole sequence, so it seemed to
fit more nicely. And I've dropped the list formatting and turned it into
prose instead.

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx