| Message ID | 20220720142732.32041-9-tzimmermann@suse.de |
|---|---|
| State | New |
| Headers | show |
| Series | drm: Add driver for PowerPC OF displays | expand |
On 7/20/22 16:27, Thomas Zimmermann wrote: > Add a dedicated CRTC state to ofdrm to later store information for > palette updates. > > Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> > --- > drivers/gpu/drm/tiny/ofdrm.c | 62 ++++++++++++++++++++++++++++++++++-- > Reviewed-by: Javier Martinez Canillas <javierm@redhat.com> [...] > +static void ofdrm_crtc_reset(struct drm_crtc *crtc) > +{ > + struct ofdrm_crtc_state *ofdrm_crtc_state; > + > + if (crtc->state) { > + ofdrm_crtc_state_destroy(to_ofdrm_crtc_state(crtc->state)); > + crtc->state = NULL; /* must be set to NULL here */ > + } > + > + ofdrm_crtc_state = kzalloc(sizeof(*ofdrm_crtc_state), GFP_KERNEL); > + if (!ofdrm_crtc_state) > + return; > + __drm_atomic_helper_crtc_reset(crtc, &ofdrm_crtc_state->base); > +} > + IMO this function is hard to read, I would instead write it as following: static void ofdrm_crtc_reset(struct drm_crtc *crtc) { struct ofdrm_crtc_state *ofdrm_crtc_state = kzalloc(sizeof(*ofdrm_crtc_state), GFP_KERNEL); if (!ofdrm_crtc_state) return; if (crtc->state) { ofdrm_crtc_state_destroy(to_ofdrm_crtc_state(crtc->state)); crtc->state = NULL; /* must be set to NULL here */ } __drm_atomic_helper_crtc_reset(crtc, &ofdrm_crtc_state->base); } Also with that form I think that the crtc->state = NULL could just be dropped ?
Hi Am 26.07.22 um 15:36 schrieb Javier Martinez Canillas: > On 7/20/22 16:27, Thomas Zimmermann wrote: >> Add a dedicated CRTC state to ofdrm to later store information for >> palette updates. >> >> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> >> --- >> drivers/gpu/drm/tiny/ofdrm.c | 62 ++++++++++++++++++++++++++++++++++-- >> > > Reviewed-by: Javier Martinez Canillas <javierm@redhat.com> > > [...] > >> +static void ofdrm_crtc_reset(struct drm_crtc *crtc) >> +{ >> + struct ofdrm_crtc_state *ofdrm_crtc_state; >> + >> + if (crtc->state) { >> + ofdrm_crtc_state_destroy(to_ofdrm_crtc_state(crtc->state)); >> + crtc->state = NULL; /* must be set to NULL here */ >> + } >> + >> + ofdrm_crtc_state = kzalloc(sizeof(*ofdrm_crtc_state), GFP_KERNEL); >> + if (!ofdrm_crtc_state) >> + return; >> + __drm_atomic_helper_crtc_reset(crtc, &ofdrm_crtc_state->base); >> +} >> + > > IMO this function is hard to read, I would instead write it as following: > > static void ofdrm_crtc_reset(struct drm_crtc *crtc) > { > struct ofdrm_crtc_state *ofdrm_crtc_state = kzalloc(sizeof(*ofdrm_crtc_state), GFP_KERNEL); > > if (!ofdrm_crtc_state) > return; > > if (crtc->state) { > ofdrm_crtc_state_destroy(to_ofdrm_crtc_state(crtc->state)); > crtc->state = NULL; /* must be set to NULL here */ > } > > __drm_atomic_helper_crtc_reset(crtc, &ofdrm_crtc_state->base); > } > > Also with that form I think that the crtc->state = NULL could just be dropped ? I once had to add this line to a driver to make the DRM helpers work. But I cannot find any longer why. Maybe it's been resolved meanwhile. Best regards Thomas >
diff --git a/drivers/gpu/drm/tiny/ofdrm.c b/drivers/gpu/drm/tiny/ofdrm.c index 6c062b48d354..ad673c9b5502 100644 --- a/drivers/gpu/drm/tiny/ofdrm.c +++ b/drivers/gpu/drm/tiny/ofdrm.c @@ -277,6 +277,21 @@ static struct resource *ofdrm_find_fb_resource(struct ofdrm_device *odev, * Modesetting */ +struct ofdrm_crtc_state { + struct drm_crtc_state base; +}; + +static struct ofdrm_crtc_state *to_ofdrm_crtc_state(struct drm_crtc_state *base) +{ + return container_of(base, struct ofdrm_crtc_state, base); +} + +static void ofdrm_crtc_state_destroy(struct ofdrm_crtc_state *ofdrm_crtc_state) +{ + __drm_atomic_helper_crtc_destroy_state(&ofdrm_crtc_state->base); + kfree(ofdrm_crtc_state); +} + /* * Support all formats of OF display and maybe more; in order * of preference. The display's update function will do any @@ -395,13 +410,54 @@ static const struct drm_crtc_helper_funcs ofdrm_crtc_helper_funcs = { .atomic_disable = ofdrm_crtc_helper_atomic_disable, }; +static void ofdrm_crtc_reset(struct drm_crtc *crtc) +{ + struct ofdrm_crtc_state *ofdrm_crtc_state; + + if (crtc->state) { + ofdrm_crtc_state_destroy(to_ofdrm_crtc_state(crtc->state)); + crtc->state = NULL; /* must be set to NULL here */ + } + + ofdrm_crtc_state = kzalloc(sizeof(*ofdrm_crtc_state), GFP_KERNEL); + if (!ofdrm_crtc_state) + return; + __drm_atomic_helper_crtc_reset(crtc, &ofdrm_crtc_state->base); +} + +static struct drm_crtc_state *ofdrm_crtc_atomic_duplicate_state(struct drm_crtc *crtc) +{ + struct drm_crtc_state *crtc_state = crtc->state; + struct ofdrm_crtc_state *ofdrm_crtc_state; + struct ofdrm_crtc_state *new_ofdrm_crtc_state; + + if (!crtc_state) + return NULL; + + ofdrm_crtc_state = to_ofdrm_crtc_state(crtc_state); + + new_ofdrm_crtc_state = kzalloc(sizeof(*new_ofdrm_crtc_state), GFP_KERNEL); + if (!new_ofdrm_crtc_state) + return NULL; + + __drm_atomic_helper_crtc_duplicate_state(crtc, &new_ofdrm_crtc_state->base); + + return &new_ofdrm_crtc_state->base; +} + +static void ofdrm_crtc_atomic_destroy_state(struct drm_crtc *crtc, + struct drm_crtc_state *crtc_state) +{ + ofdrm_crtc_state_destroy(to_ofdrm_crtc_state(crtc_state)); +} + static const struct drm_crtc_funcs ofdrm_crtc_funcs = { - .reset = drm_atomic_helper_crtc_reset, + .reset = ofdrm_crtc_reset, .destroy = drm_crtc_cleanup, .set_config = drm_atomic_helper_set_config, .page_flip = drm_atomic_helper_page_flip, - .atomic_duplicate_state = drm_atomic_helper_crtc_duplicate_state, - .atomic_destroy_state = drm_atomic_helper_crtc_destroy_state, + .atomic_duplicate_state = ofdrm_crtc_atomic_duplicate_state, + .atomic_destroy_state = ofdrm_crtc_atomic_destroy_state, }; static int ofdrm_connector_helper_get_modes(struct drm_connector *connector)
Add a dedicated CRTC state to ofdrm to later store information for palette updates. Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> --- drivers/gpu/drm/tiny/ofdrm.c | 62 ++++++++++++++++++++++++++++++++++-- 1 file changed, 59 insertions(+), 3 deletions(-)