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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Tue, 12 Sep 2023 11:03:50 +0200
From:   Javier Martinez Canillas <javierm@...hat.com>
To:     Maxime Ripard <mripard@...nel.org>
Cc:     linux-kernel@...r.kernel.org,
        Geert Uytterhoeven <geert@...ux-m68k.org>,
        Thomas Zimmermann <tzimmermann@...e.de>,
        Daniel Vetter <daniel@...ll.ch>,
        David Airlie <airlied@...il.com>,
        dri-devel@...ts.freedesktop.org
Subject: Re: [PATCH v2] drm/ssd130x: Store the HW buffer in the
 driver-private CRTC state

Maxime Ripard <mripard@...nel.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