Maxime Ripard <mrip...@kernel.org> 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
