Re: [PATCH 2/9] drm/tegra: Assign conn_state->connector when allocating connector state.

[Thierry Reding <thierry.reding@gmail.com>]

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

On Tue, Nov 24, 2015 at 11:37:47AM +0100, Daniel Vetter wrote:
> On Tue, Nov 24, 2015 at 10:34:29AM +0100, Maarten Lankhorst wrote:
> > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > ---
> >  drivers/gpu/drm/tegra/dsi.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/tegra/dsi.c b/drivers/gpu/drm/tegra/dsi.c
> > index f0a138ef68ce..6b8ae3d08eeb 100644
> > --- a/drivers/gpu/drm/tegra/dsi.c
> > +++ b/drivers/gpu/drm/tegra/dsi.c
> > @@ -751,8 +751,10 @@ static void tegra_dsi_connector_reset(struct drm_connector *connector)
> >  	connector->state = NULL;
> >  
> >  	state = kzalloc(sizeof(*state), GFP_KERNEL);
> > -	if (state)
> > +	if (state) {
> > +		state->base.connector = connector;
> >  		connector->state = &state->base;
> > +	}
> 
> Hm, we might want __ versions of all the _reset hooks if this becomes more
> common. I do wonder a bit why it isn't since a lot of drivers overwrite
> state structures by now, and then the provided reset functions aren't
> sufficient really.

We already have the __ versions for duplicate_state helpers, but the
problem with reset helpers is that they need to know the allocation
size...

Actually, that's true of the duplicate_state helpers as well, and the __
variants don't actually allocate the memory but rather just copy the
state from old to new. So we could do something just like that for the
reset helpers:

	void __drm_atomic_helper_connector_reset(struct drm_connector *connector,
						 struct drm_connector_state *state)
	{
		state->base.connector = connector;
		connector->state = state;
	}

and use like this:

	static void tegra_dsi_connector_reset(struct drm_connector *connector)
	{
		struct tegra_dsi_state *state;
		...
		state = kzalloc(sizeof(*state), GFP_KERNEL);
		if (state)
			__drm_atomic_helper_connector_reset(connector, &state->base);
	}

I think back at the time when we did this for duplicate_state helpers we
had a discussion about the usefulness of this, but this patchset clearly
shows that this kind of change, which applies to a number of drivers, is
going to happen again and again, so the only way to avoid mistakes or an
extensive set of changes across all drivers is by putting this into a
helper, even if it's only two assignments now.

Thierry

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 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