| Message ID | 1448357676-27837-3-git-send-email-maarten.lankhorst@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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(-) Acked-by: Thierry Reding <treding@nvidia.com>
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. -Daniel
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
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
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; + } } static struct drm_connector_state *
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> --- drivers/gpu/drm/tegra/dsi.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)