Message-ID: <6740c736-ba5f-4df2-ba2b-da0902f51f6d@suse.de>
Date:   Fri, 13 Oct 2023 09:35:31 +0200
From:   Thomas Zimmermann <tzimmermann@...e.de>
To:     Javier Martinez Canillas <javierm@...hat.com>,
        linux-kernel@...r.kernel.org
Cc:     Conor Dooley <conor@...nel.org>, dri-devel@...ts.freedesktop.org,
        Maxime Ripard <mripard@...nel.org>,
        Geert Uytterhoeven <geert@...ux-m68k.org>,
        Peter Robinson <pbrobinson@...il.com>
Subject: Re: [PATCH v3 4/6] drm/ssd130x: Add support for the SSD132x OLED
 controller family

Hi Javier,

thanks for this patch.

Am 12.10.23 um 23:38 schrieb Javier Martinez Canillas:
[...]
>   
> +static int ssd132x_fb_blit_rect(struct drm_framebuffer *fb,
> +				const struct iosys_map *vmap,
> +				struct drm_rect *rect, u8 *buf,
> +				u8 *data_array)
> +{
> +	struct ssd130x_device *ssd130x = drm_to_ssd130x(fb->dev);
> +	unsigned int dst_pitch = drm_rect_width(rect);
> +	struct iosys_map dst;
> +	int ret = 0;
> +
> +	/* Align x to display segment boundaries */
> +	rect->x1 = round_down(rect->x1, SSD132X_SEGMENT_WIDTH);
> +	rect->x2 = min_t(unsigned int, round_up(rect->x2, SSD132X_SEGMENT_WIDTH),
> +			 ssd130x->width);
> +
> +	ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE);
> +	if (ret)
> +		return ret;
> +
> +	iosys_map_set_vaddr(&dst, buf);
> +	drm_fb_xrgb8888_to_gray8(&dst, &dst_pitch, vmap, fb, rect);

Here's an idea for a follow-up patchset.

You could attempt to integrate the gray8 and mono conversions into 
drm_fb_blit(). With some the right parameters, both, ssd130x and ssd132x 
could use the same blitting code from BO to buffer.

> +
> +	drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE);
> +
> +	ssd132x_update_rect(ssd130x, rect, buf, data_array);
> +
> +	return ret;
> +}
> +
>   static int ssd130x_primary_plane_atomic_check(struct drm_plane *plane,
>   					      struct drm_atomic_state *state)
>   {
> @@ -677,6 +901,43 @@ static int ssd130x_primary_plane_atomic_check(struct drm_plane *plane,
>   	return 0;
>   }
>   
> +static int ssd132x_primary_plane_atomic_check(struct drm_plane *plane,
> +					      struct drm_atomic_state *state)
> +{
> +	struct drm_device *drm = plane->dev;
> +	struct ssd130x_device *ssd130x = drm_to_ssd130x(drm);
> +	struct drm_plane_state *plane_state = drm_atomic_get_new_plane_state(state, plane);
> +	struct ssd130x_plane_state *ssd130x_state = to_ssd130x_plane_state(plane_state);
> +	struct drm_crtc *crtc = plane_state->crtc;
> +	struct drm_crtc_state *crtc_state;
> +	const struct drm_format_info *fi;
> +	unsigned int pitch;
> +	int ret;
> +
> +	if (!crtc)
> +		return -EINVAL;
> +
> +	crtc_state = drm_atomic_get_crtc_state(state, crtc);
> +	if (IS_ERR(crtc_state))
> +		return PTR_ERR(crtc_state);
> +
> +	ret = drm_plane_helper_atomic_check(plane, state);
> +	if (ret)
> +		return ret;
> +
> +	fi = drm_format_info(DRM_FORMAT_R8);
> +	if (!fi)
> +		return -EINVAL;
> +
> +	pitch = drm_format_info_min_pitch(fi, 0, ssd130x->width);
> +
> +	ssd130x_state->buffer = kcalloc(pitch, ssd130x->height, GFP_KERNEL);
> +	if (!ssd130x_state->buffer)
> +		return -ENOMEM;

It's unrelated to these patches and I know it's been discussed 
endlessly, but I have a questions about buffer allocation. That memory 
acts as another shadow buffer for the device's memory, such that format 
conversion becomes easier.

But then, why is ->buffer part of the plane_state? Shouldn't it be part 
of the plane and never be re-allocated? The real size of that buffer is 
<width> times <height> (not <pitch>). That size is static over the 
lifetime of the device. That would represent the semantics much better.

This would allow for additional changes: blit_rect and update_rect would 
be much easier to separate: no more segment adjustments for the blit 
code; only for updates. If the update code has high latency (IDK), you 
could push it into a worker thread to run besides the DRM logic. The gud 
and repaper drivers do something to this effect.


> +
> +	return 0;
> +}
> +
>   static void ssd130x_primary_plane_atomic_update(struct drm_plane *plane,
>   						struct drm_atomic_state *state)
>   {
> @@ -711,6 +972,40 @@ static void ssd130x_primary_plane_atomic_update(struct drm_plane *plane,
>   	drm_dev_exit(idx);
>   }
>   
> +static void ssd132x_primary_plane_atomic_update(struct drm_plane *plane,
> +						struct drm_atomic_state *state)
> +{
> +	struct drm_plane_state *plane_state = drm_atomic_get_new_plane_state(state, plane);
> +	struct drm_plane_state *old_plane_state = drm_atomic_get_old_plane_state(state, plane);
> +	struct drm_shadow_plane_state *shadow_plane_state = to_drm_shadow_plane_state(plane_state);
> +	struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(state, plane_state->crtc);
> +	struct ssd130x_crtc_state *ssd130x_crtc_state =  to_ssd130x_crtc_state(crtc_state);
> +	struct ssd130x_plane_state *ssd130x_plane_state = to_ssd130x_plane_state(plane_state);
> +	struct drm_framebuffer *fb = plane_state->fb;
> +	struct drm_atomic_helper_damage_iter iter;
> +	struct drm_device *drm = plane->dev;
> +	struct drm_rect dst_clip;
> +	struct drm_rect damage;
> +	int idx;
> +
> +	if (!drm_dev_enter(drm, &idx))
> +		return;
> +
> +	drm_atomic_helper_damage_iter_init(&iter, old_plane_state, plane_state);
> +	drm_atomic_for_each_plane_damage(&iter, &damage) {
> +		dst_clip = plane_state->dst;
> +
> +		if (!drm_rect_intersect(&dst_clip, &damage))
> +			continue;
> +
> +		ssd132x_fb_blit_rect(fb, &shadow_plane_state->data[0], &dst_clip,
> +				     ssd130x_plane_state->buffer,
> +				     ssd130x_crtc_state->data_array);
> +	}

Here's another idea for a another follow-up patchset:

You are allocating state->buffer to cover the whole display, right? It's 
<pitch> times <height> IIRC.  Maybe it would make sense to split the 
damage loop into two loops and inline the driver's blit_rect() function. 
Something like that

   begin_cpu_access()

   for_each(damage) {
     drm_fb_blit( "from GEM BO to buffer" )
   }

   end_cpu_access()

   for_each(damge) {
     update_rect( "from buffer to device" )
   }

With the changes from the other comments, the first loop could become 
entirely device-neutral AFAICT.

Best regards
Thomas

> +
> +	drm_dev_exit(idx);
> +}
> +
>   static void ssd130x_primary_plane_atomic_disable(struct drm_plane *plane,
>   						 struct drm_atomic_state *state)
>   {
> @@ -735,6 +1030,30 @@ static void ssd130x_primary_plane_atomic_disable(struct drm_plane *plane,
>   	drm_dev_exit(idx);
>   }
>   
> +static void ssd132x_primary_plane_atomic_disable(struct drm_plane *plane,
> +						 struct drm_atomic_state *state)
> +{
> +	struct drm_device *drm = plane->dev;
> +	struct ssd130x_device *ssd130x = drm_to_ssd130x(drm);
> +	struct drm_plane_state *plane_state = drm_atomic_get_new_plane_state(state, plane);
> +	struct drm_crtc_state *crtc_state;
> +	struct ssd130x_crtc_state *ssd130x_crtc_state;
> +	int idx;
> +
> +	if (!plane_state->crtc)
> +		return;
> +
> +	crtc_state = drm_atomic_get_new_crtc_state(state, plane_state->crtc);
> +	ssd130x_crtc_state = to_ssd130x_crtc_state(crtc_state);
> +
> +	if (!drm_dev_enter(drm, &idx))
> +		return;
> +
> +	ssd132x_clear_screen(ssd130x, ssd130x_crtc_state->data_array);
> +
> +	drm_dev_exit(idx);
> +}
> +
>   /* Called during init to allocate the plane's atomic state. */
>   static void ssd130x_primary_plane_reset(struct drm_plane *plane)
>   {
> @@ -785,11 +1104,19 @@ static void ssd130x_primary_plane_destroy_state(struct drm_plane *plane,
>   	kfree(ssd130x_state);
>   }
>   
> -static const struct drm_plane_helper_funcs ssd130x_primary_plane_helper_funcs = {
> -	DRM_GEM_SHADOW_PLANE_HELPER_FUNCS,
> -	.atomic_check = ssd130x_primary_plane_atomic_check,
> -	.atomic_update = ssd130x_primary_plane_atomic_update,
> -	.atomic_disable = ssd130x_primary_plane_atomic_disable,
> +static const struct drm_plane_helper_funcs ssd130x_primary_plane_helper_funcs[] = {
> +	[SSD130X_FAMILY] = {
> +		DRM_GEM_SHADOW_PLANE_HELPER_FUNCS,
> +		.atomic_check = ssd130x_primary_plane_atomic_check,
> +		.atomic_update = ssd130x_primary_plane_atomic_update,
> +		.atomic_disable = ssd130x_primary_plane_atomic_disable,
> +	},
> +	[SSD132X_FAMILY] = {
> +		DRM_GEM_SHADOW_PLANE_HELPER_FUNCS,
> +		.atomic_check = ssd132x_primary_plane_atomic_check,
> +		.atomic_update = ssd132x_primary_plane_atomic_update,
> +		.atomic_disable = ssd132x_primary_plane_atomic_disable,
> +	}
>   };
>   
>   static const struct drm_plane_funcs ssd130x_primary_plane_funcs = {
> @@ -838,6 +1165,27 @@ static int ssd130x_crtc_atomic_check(struct drm_crtc *crtc,
>   	return 0;
>   }
>   
> +static int ssd132x_crtc_atomic_check(struct drm_crtc *crtc,
> +				     struct drm_atomic_state *state)
> +{
> +	struct drm_device *drm = crtc->dev;
> +	struct ssd130x_device *ssd130x = drm_to_ssd130x(drm);
> +	struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
> +	struct ssd130x_crtc_state *ssd130x_state = to_ssd130x_crtc_state(crtc_state);
> +	unsigned int columns = DIV_ROUND_UP(ssd130x->width, SSD132X_SEGMENT_WIDTH);
> +	int ret;
> +
> +	ret = drm_crtc_helper_atomic_check(crtc, state);
> +	if (ret)
> +		return ret;
> +
> +	ssd130x_state->data_array = kmalloc(columns * ssd130x->height, GFP_KERNEL);
> +	if (!ssd130x_state->data_array)
> +		return -ENOMEM;
> +
> +	return 0;
> +}
> +
>   /* Called during init to allocate the CRTC's atomic state. */
>   static void ssd130x_crtc_reset(struct drm_crtc *crtc)
>   {
> @@ -890,9 +1238,15 @@ static void ssd130x_crtc_destroy_state(struct drm_crtc *crtc,
>    * the primary plane's atomic_update function. Disabling clears
>    * the screen in the primary plane's atomic_disable function.
>    */
> -static const struct drm_crtc_helper_funcs ssd130x_crtc_helper_funcs = {
> -	.mode_valid = ssd130x_crtc_mode_valid,
> -	.atomic_check = ssd130x_crtc_atomic_check,
> +static const struct drm_crtc_helper_funcs ssd130x_crtc_helper_funcs[] = {
> +	[SSD130X_FAMILY] = {
> +		.mode_valid = ssd130x_crtc_mode_valid,
> +		.atomic_check = ssd130x_crtc_atomic_check,
> +	},
> +	[SSD132X_FAMILY] = {
> +		.mode_valid = ssd130x_crtc_mode_valid,
> +		.atomic_check = ssd132x_crtc_atomic_check,
> +	},
>   };
>   
>   static const struct drm_crtc_funcs ssd130x_crtc_funcs = {
> @@ -930,6 +1284,31 @@ static void ssd130x_encoder_atomic_enable(struct drm_encoder *encoder,
>   	return;
>   }
>   
> +static void ssd132x_encoder_atomic_enable(struct drm_encoder *encoder,
> +					  struct drm_atomic_state *state)
> +{
> +	struct drm_device *drm = encoder->dev;
> +	struct ssd130x_device *ssd130x = drm_to_ssd130x(drm);
> +	int ret;
> +
> +	ret = ssd130x_power_on(ssd130x);
> +	if (ret)
> +		return;
> +
> +	ret = ssd132x_init(ssd130x);
> +	if (ret)
> +		goto power_off;
> +
> +	ssd130x_write_cmd(ssd130x, 1, SSD13XX_DISPLAY_ON);
> +
> +	backlight_enable(ssd130x->bl_dev);
> +
> +	return;
> +
> +power_off:
> +	ssd130x_power_off(ssd130x);
> +}
> +
>   static void ssd130x_encoder_atomic_disable(struct drm_encoder *encoder,
>   					   struct drm_atomic_state *state)
>   {
> @@ -943,9 +1322,15 @@ static void ssd130x_encoder_atomic_disable(struct drm_encoder *encoder,
>   	ssd130x_power_off(ssd130x);
>   }
>   
> -static const struct drm_encoder_helper_funcs ssd130x_encoder_helper_funcs = {
> -	.atomic_enable = ssd130x_encoder_atomic_enable,
> -	.atomic_disable = ssd130x_encoder_atomic_disable,
> +static const struct drm_encoder_helper_funcs ssd130x_encoder_helper_funcs[] = {
> +	[SSD130X_FAMILY] = {
> +		.atomic_enable = ssd130x_encoder_atomic_enable,
> +		.atomic_disable = ssd130x_encoder_atomic_disable,
> +	},
> +	[SSD132X_FAMILY] = {
> +		.atomic_enable = ssd132x_encoder_atomic_enable,
> +		.atomic_disable = ssd130x_encoder_atomic_disable,
> +	}
>   };
>   
>   static const struct drm_encoder_funcs ssd130x_encoder_funcs = {
> @@ -1079,6 +1464,7 @@ static void ssd130x_parse_properties(struct ssd130x_device *ssd130x)
>   
>   static int ssd130x_init_modeset(struct ssd130x_device *ssd130x)
>   {
> +	enum ssd130x_family_ids family_id = ssd130x->device_info->family_id;
>   	struct drm_display_mode *mode = &ssd130x->mode;
>   	struct device *dev = ssd130x->dev;
>   	struct drm_device *drm = &ssd130x->drm;
> @@ -1129,7 +1515,7 @@ static int ssd130x_init_modeset(struct ssd130x_device *ssd130x)
>   		return ret;
>   	}
>   
> -	drm_plane_helper_add(primary_plane, &ssd130x_primary_plane_helper_funcs);
> +	drm_plane_helper_add(primary_plane, &ssd130x_primary_plane_helper_funcs[family_id]);
>   
>   	drm_plane_enable_fb_damage_clips(primary_plane);
>   
> @@ -1143,7 +1529,7 @@ static int ssd130x_init_modeset(struct ssd130x_device *ssd130x)
>   		return ret;
>   	}
>   
> -	drm_crtc_helper_add(crtc, &ssd130x_crtc_helper_funcs);
> +	drm_crtc_helper_add(crtc, &ssd130x_crtc_helper_funcs[family_id]);
>   
>   	/* Encoder */
>   
> @@ -1155,7 +1541,7 @@ static int ssd130x_init_modeset(struct ssd130x_device *ssd130x)
>   		return ret;
>   	}
>   
> -	drm_encoder_helper_add(encoder, &ssd130x_encoder_helper_funcs);
> +	drm_encoder_helper_add(encoder, &ssd130x_encoder_helper_funcs[family_id]);
>   
>   	encoder->possible_crtcs = drm_crtc_mask(crtc);
>   
> diff --git a/drivers/gpu/drm/solomon/ssd130x.h b/drivers/gpu/drm/solomon/ssd130x.h
> index a5a25e054d2f..acf7cedf0c1a 100644
> --- a/drivers/gpu/drm/solomon/ssd130x.h
> +++ b/drivers/gpu/drm/solomon/ssd130x.h
> @@ -25,7 +25,8 @@
>   #define SSD13XX_COMMAND				0x80
>   
>   enum ssd130x_family_ids {
> -	SSD130X_FAMILY
> +	SSD130X_FAMILY,
> +	SSD132X_FAMILY
>   };
>   
>   enum ssd130x_variants {
> @@ -35,6 +36,10 @@ enum ssd130x_variants {
>   	SSD1306_ID,
>   	SSD1307_ID,
>   	SSD1309_ID,
> +	/* ssd132x family */
> +	SSD1322_ID,
> +	SSD1325_ID,
> +	SSD1327_ID,
>   	NR_SSD130X_VARIANTS
>   };
>   

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

Download attachment "OpenPGP_signature.asc" of type "application/pgp-signature" (841 bytes)

Powered by blists - more mailing lists