Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.32 as permitted sender) client-ip=23.128.96.32;
From:   Javier Martinez Canillas <javierm@redhat.com>
To:     Maxime Ripard <mripard@kernel.org>
Cc:     linux-kernel@vger.kernel.org,
        Geert Uytterhoeven <geert@linux-m68k.org>,
        Thomas Zimmermann <tzimmermann@suse.de>,
        Daniel Vetter <daniel@ffwll.ch>,
        David Airlie <airlied@gmail.com>,
        dri-devel@lists.freedesktop.org
Subject: Re: [PATCH v2] drm/ssd130x: Store the HW buffer in the
 driver-private CRTC state
In-Reply-To: <x75lakzrl2awslaid5ffneepxusej7b7ojvz36bgpdwriztcru@h4bvwr2oeaov>
References: <20230910094041.715408-1-javierm@redhat.com>
 <x75lakzrl2awslaid5ffneepxusej7b7ojvz36bgpdwriztcru@h4bvwr2oeaov>
Date:   Tue, 12 Sep 2023 11:03:50 +0200
Message-ID: <874jjz3gqx.fsf@minerva.mail-host-address-is-not-set>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

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