diff mbox series

[v2,08/10] drm/ofdrm: Add CRTC state

Message ID 20220720142732.32041-9-tzimmermann@suse.de
State New
Headers show
Series drm: Add driver for PowerPC OF displays | expand

Commit Message

Thomas Zimmermann July 20, 2022, 2:27 p.m. UTC
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(-)

Comments

Javier Martinez Canillas July 26, 2022, 1:36 p.m. UTC | #1
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 ?
Thomas Zimmermann Sept. 21, 2022, 11:45 a.m. UTC | #2
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 mbox series

Patch

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)