[v3,4/6] drm/ssd130x: Add support for the SSD132x OLED controller family

  The Solomon SSD132x controllers (such as the SSD1322, SSD1325 and SSD1327)
are used by 16 grayscale dot matrix OLED panels, extend the driver to also
support this chip family.

Instead adding an indirection level to allow the same modesetting pipeline
to be used by both controller families, add another pipeline for SSD132x.

This leads to some code duplication but it makes the driver easier to read
and understand. Once other controller families are added (e.g: SSD133x),
some common code can be factored out in driver helpers to be shared by the
different families. But that can be done later once these patterns emerge.

Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
---

Changes in v3:
- Use different modesetting pipelines for chip families (Thomas Zimmermann).
- Change the i,j variables type to unsigned int (Geert Uytterhoeven).

Changes in v2:
- Align the rectangle to the segment width (Geert Uytterhoeven).

 drivers/gpu/drm/solomon/Kconfig       |  12 +-
 drivers/gpu/drm/solomon/ssd130x-i2c.c |  17 +-
 drivers/gpu/drm/solomon/ssd130x-spi.c |  21 +-
 drivers/gpu/drm/solomon/ssd130x.c     | 418 +++++++++++++++++++++++++-
 drivers/gpu/drm/solomon/ssd130x.h     |   7 +-
 5 files changed, 448 insertions(+), 27 deletions(-)
  

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)
  

Thomas Zimmermann <tzimmermann@suse.de> writes:

Hello Thomas,

Thanks a lot for your feedback.

> 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.
>

Yeah, I considered that but as mentioned in the commit message want to see
what are the needs of the SSD133x controller family (I bought a SSD1331
display but haven't had time to play with it yet) before trying to factor
out the common bits in helper functions.

[...]

>> +
>> +	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.
>

Correct.

> 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.
>
>

The idea of making it part of the plane state is that this buffer could be
optional, for example in the case of user-space using the native display
format instead of the emulated XRGB8888.

In that case, an intermediate buffer won't be used because the shadow-plane
format will already be the native one (e.g: R1) and there won't be a need
to do any format conversion (only the conversion to the data format as is
expected by the controller).

Take a look to Geert's patch adding R1 support to ssd130x for an example:

https://lore.kernel.org/all/72746f6d9c47f09fc057ad7a4bbb3b7f423af803.1689252746.git.geert@linux-m68k.org/

That's why it was decided that making it part of the plane state follows
better the KMS model, because when using R1 this buffer won't even be
allocated in the primary plane .atomic_check handler.

[...]

>> +	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.
>

Regardless, splitting the blit and update rect might make sense and is an
intersesting idea. I need to explore this, thanks for the suggestion.

As you mention that these could be follow-up changes, I assume that you
agree with the current approach. Should I expect your review / ack for
this patch-set?

> Best regards
> Thomas
>
  

Hi

Am 13.10.23 um 16:57 schrieb Javier Martinez Canillas:
> Thomas Zimmermann <tzimmermann@suse.de> writes:
> 
> Hello Thomas,
> 
> Thanks a lot for your feedback.
> 
>> 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.
>>
> 
> Yeah, I considered that but as mentioned in the commit message want to see
> what are the needs of the SSD133x controller family (I bought a SSD1331
> display but haven't had time to play with it yet) before trying to factor
> out the common bits in helper functions.
> 
> [...]
> 
>>> +
>>> +	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.
>>
> 
> Correct.
> 
>> 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.
>>
>>
> 
> The idea of making it part of the plane state is that this buffer could be
> optional, for example in the case of user-space using the native display
> format instead of the emulated XRGB8888.
> 
> In that case, an intermediate buffer won't be used because the shadow-plane
> format will already be the native one (e.g: R1) and there won't be a need
> to do any format conversion (only the conversion to the data format as is
> expected by the controller).
> 
> Take a look to Geert's patch adding R1 support to ssd130x for an example:
> 
> https://lore.kernel.org/all/72746f6d9c47f09fc057ad7a4bbb3b7f423af803.1689252746.git.geert@linux-m68k.org/
> 
> That's why it was decided that making it part of the plane state follows
> better the KMS model, because when using R1 this buffer won't even be
> allocated in the primary plane .atomic_check handler.
> 
> [...]
> 
>>> +	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.
>>
> 
> Regardless, splitting the blit and update rect might make sense and is an
> intersesting idea. I need to explore this, thanks for the suggestion.
> 
> As you mention that these could be follow-up changes, I assume that you
> agree with the current approach. Should I expect your review / ack for
> this patch-set?

Please take my ack for this patchset

Acked-by: Thomas Zimmermann <tzimmermann@suse.de>

Best regards
Thomas

> 
>> Best regards
>> Thomas
>>
> 

-- 
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)
  

Thomas Zimmermann <tzimmermann@suse.de> writes:

Hello Thomas,

[...]
 
>> As you mention that these could be follow-up changes, I assume that you
>> agree with the current approach. Should I expect your review / ack for
>> this patch-set?
>
> Please take my ack for this patchset
>
> Acked-by: Thomas Zimmermann <tzimmermann@suse.de>
>

Perfect, thanks a lot!

> Best regards
> Thomas
>