Re: [PATCH v2 08/10] drm/ofdrm: Add CRTC state
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
---
drivers/gpu/drm/tiny/ofdrm.c | 62 ++--
Reviewed-by: Javier Martinez Canillas
[...]
+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
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev
OpenPGP_signature
Description: OpenPGP digital signature
Re: [PATCH v2 08/10] drm/ofdrm: Add CRTC state
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
> ---
> drivers/gpu/drm/tiny/ofdrm.c | 62 ++--
>
Reviewed-by: Javier Martinez Canillas
[...]
> +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 ?
--
Best regards,
Javier Martinez Canillas
Linux Engineering
Red Hat
[PATCH v2 08/10] drm/ofdrm: Add CRTC state
Add a dedicated CRTC state to ofdrm to later store information for
palette updates.
Signed-off-by: Thomas Zimmermann
---
drivers/gpu/drm/tiny/ofdrm.c | 62 ++--
1 file changed, 59 insertions(+), 3 deletions(-)
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)
--
2.36.1
