Hi

Am 05.09.22 um 13:00 schrieb Javier Martinez Canillas:
> Hello Thomas,
> 
> Thanks for your feedback and comments.
> 
> On 9/5/22 12:41, Thomas Zimmermann wrote:
>> Hi Javier
>>
>> Am 28.08.22 um 17:11 schrieb Javier Martinez Canillas:
>>> The simple display pipeline is a set of helpers that can be used by DRM
>>> drivers to avoid dealing with all the needed components and just define
>>> a few functions to operate a simple display device with one full-screen
>>> scanout buffer feeding a single output.
>>>
>>> But it is arguable that this provides the correct level of abstraction
>>> for simple drivers, and recently some have been ported from using these
>>> simple display helpers to use the regular atomic helpers instead.
>>>
>>> The rationale for this is that the simple display pipeline helpers don't
>>> hide that much of the DRM complexity, while adding an indirection layer
>>> that conflates the concepts of CRTCs and planes. This makes the helpers
>>> less flexible and harder to be reused among different graphics drivers.
>>>
>>> Also, for simple drivers, using the full atomic helpers doesn't require
>>> a lot of additional code. So adding a simple display pipeline layer may
>>> not be worth it.
>>>
>>> For these reasons, let's follow that trend and make ssd130x a plain DRM
>>> driver that creates its own primary plane, CRTC, enconder and connector.
>>
>> Thanks for considering this change.
>>
> 
> You are welcome and thanks to you for mentioning this to me. After doing
> this I'm convinced as well that the simple-KMS / simple display pipeline
> abstraction doesn't add any value and we should just drop it in favor of
> the full atomic helpers.
> 
>>>
>>> Suggested-by: Thomas Zimmermann <tzimmermann@suse.de>
>>> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
>>
>> Acked-by: Thomas Zimmermann <tzimmermann@suse.de>
>>
> 
> Thanks!
>   
>> There are a few questions below.
>>
> 
> [...]
> 
>>> +static void ssd130x_primary_plane_helper_atomic_update(struct drm_plane *plane,
>>> +						       struct drm_atomic_state *old_state)
>>>    {
>>> -	struct ssd130x_device *ssd130x = drm_to_ssd130x(pipe->crtc.dev);
>>> +	struct drm_plane_state *plane_state = plane->state;
>>> +	struct drm_plane_state *old_plane_state = drm_atomic_get_old_plane_state(old_state, plane);
>>>    	struct drm_shadow_plane_state *shadow_plane_state = to_drm_shadow_plane_state(plane_state);
>>> -	struct drm_device *drm = &ssd130x->drm;
>>> -	int idx, ret;
>>> +	struct drm_framebuffer *fb = plane_state->fb;
>>> +	struct drm_device *drm = plane->dev;
>>> +	struct drm_rect src_clip, dst_clip;
>>> +	int idx;
>>>    
>>> -	ret = ssd130x_power_on(ssd130x);
>>> -	if (ret)
>>> +	if (!fb)
>>
>> I know that some other drivers do this check. But from reading
>> drm_atomic_helper.c, it shouldn't be necesary. If !fb, the plane has
>> been disabled. And because there's an implementation of atomic_disable,
>> the helpers should never call atomic_update on disabled planes. I think
>> the test can be removed.
>>
> 
> Yes, I just added because noticed that others drivers did. I'll drop it
> when posting a v2.
> 
> [...]
> 
>>> +static void ssd130x_encoder_helper_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;
>>>    
>>> -	ssd130x_fb_blit_rect(plane_state->fb, &shadow_plane_state->data[0], &dst_clip);
>>> +	ret = ssd130x_init(ssd130x);
>>> +	if (ret)
>>> +		return ssd130x_power_off(ssd130x);
>>
>> It returns a value from a function returning 'void'?
>>
> 
> Right. I'll fix it in v2 as well.
>   
>> Is this function the correct place for ssd130x_init() ? It looks a bit
>> heavy for a simple enable operation.
>>
> 
> Yes, I was abusing the concept of encoder here just to have a place where
> I could hook the enable / disable logic, since I was looking at the other
> DRM objects helper operations structures and found that these were only
> defined for the encoder.

I liked the idea of handling backlighting here. Power on/off also seems 
sensible.

> 
> But there is technically no encoder on this device. As you can see, I was
> using DRM_MODE_ENCODER_NONE when the encoder is initialized.
> 
> But I notice now that the struct drm_crtc_helper_funcs also have .enable
> and .disable callbacks, it seems I was just blind and didn't see before.

You certainly want to use atomic_enable/atomic_disable. They are 
mutually exclusive with the other enable/disable functions.

> 
> Would having the init and poweroff logic in the CRTC helpers be correct
> to you or was do you have in mind ?

There's quite a bit happening in the init function. Does it have to be 
re-initialized on each enable operation?  If it survives the power-off 
call, the initial init can be done in the CRTC reset function. It's 
purpose is to set hardware and software to a clean state.

Best regards
Thomas

> 
>> Best regards
>> Thomas

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev