Re: [PATCH v2] drm/ssd130x: Store the HW buffer in the driver-private CRTC state

[Javier Martinez Canillas]

Maxime Ripard <mripard@xxxxxxxxxx> writes:

Hello Maxime,

Thanks for your feedback!

> Hi,
>
> On Sun, Sep 10, 2023 at 11:40:28AM +0200, Javier Martinez Canillas wrote:

[...]

>>  static int ssd130x_update_rect(struct ssd130x_device *ssd130x,
>> -			       struct ssd130x_plane_state *ssd130x_state,
>> -			       struct drm_rect *rect)
>> +			       struct drm_rect *rect, u8 *buf,
>> +			       u8 *data_array)
>>  {
>>  	unsigned int x = rect->x1;
>>  	unsigned int y = rect->y1;
>> -	u8 *buf = ssd130x_state->buffer;
>> -	u8 *data_array = ssd130x_state->data_array;
>
> That's just a matter of taste I guess, but I would pass the crtc_state
> pointer there instead of an opaque byte array (without any boundary).
>

Interesting that you mentioned, I was actually on the fence on this. The
reason why I passed an opaque byte array was that Geert had it in his R1
series and wanted to align with the changes that he was doing in that set:

https://lists.freedesktop.org/archives/dri-devel/2023-August/419935.html

But I'm OK of with passing the state pointers instead. BTW, you said
crtc_state but is plane_state since the function uses both buffers.

[...]

>>  
>> @@ -671,6 +664,10 @@ static void ssd130x_primary_plane_helper_atomic_update(struct drm_plane *plane,
>>  	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);
>
> You can have CRTC-less commits if you only modify a property of the
> plane for example. drm_atomic_get_new_crtc_state will retrieve the CRTC
> state in the global state passed as an argument, but will not make any
> effort to retrieve the current CRTC state if it's not part of that commit.
>

Oh, I see. I wasn't aware of this.

> You should add a call to drm_atomic_get_crtc_state in your atomic_check
> hook to pull the CRTC state if it's not there so you can rely on it
> here.
>

Got it. I'll fix that in v2.

> Maxime

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat