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

[Maarten Lankhorst <maarten.lankhorst@linux.intel.com>]

Op 24-11-15 om 11:51 schreef Thierry Reding:
> 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.
Yeah was considering it, but it felt a bit overkill for something that only holds a pointer to crtc, best_encoder and connector..

If this is the way to go I'll resend
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx