[PATCH] drm: renesas: shmobile: Add drm_panic support

[Jocelyn Falempe]

On 29/05/2024 15:33, Laurent Pinchart wrote:
> On Wed, May 29, 2024 at 04:28:44PM +0300, Dmitry Baryshkov wrote:
>> On Wed, May 29, 2024 at 12:55:06PM +0300, Laurent Pinchart wrote:
>>> On Wed, May 29, 2024 at 12:25:56PM +0300, Dmitry Baryshkov wrote:
>>>> On Wed, May 29, 2024 at 12:10:18PM +0300, Laurent Pinchart wrote:
>>>>> Hi Dmitry,
>>>>>
>>>>> On Wed, May 29, 2024 at 11:27:02AM +0300, Dmitry Baryshkov wrote:
>>>>>> On Wed, May 29, 2024 at 04:03:20AM +0300, Laurent Pinchart wrote:
>>>>>>> On Mon, May 27, 2024 at 03:34:48PM +0200, Geert Uytterhoeven wrote:
>>>>>>>> Add support for the drm_panic module, which displays a message on
>>>>>>>> the screen when a kernel panic occurs.
>>>>>>>>
>>>>>>>> Signed-off-by: Geert Uytterhoeven <geert+renesas at glider.be>
>>>>>>>> ---
>>>>>>>> Tested on Armadillo-800-EVA.
>>>>>>>> ---
>>>>>>>>   drivers/gpu/drm/renesas/shmobile/shmob_drm_plane.c | 14 +++++++++++++-
>>>>>>>>   1 file changed, 13 insertions(+), 1 deletion(-)
>>>>>>>>
>>>>>>>> diff --git a/drivers/gpu/drm/renesas/shmobile/shmob_drm_plane.c b/drivers/gpu/drm/renesas/shmobile/shmob_drm_plane.c
>>>>>>>> index 07ad17d24294d5e6..9d166ab2af8bd231 100644
>>>>>>>> --- a/drivers/gpu/drm/renesas/shmobile/shmob_drm_plane.c
>>>>>>>> +++ b/drivers/gpu/drm/renesas/shmobile/shmob_drm_plane.c
>>>>>>>> @@ -273,6 +273,13 @@ static const struct drm_plane_helper_funcs shmob_drm_plane_helper_funcs = {
>>>>>>>>   	.atomic_disable = shmob_drm_plane_atomic_disable,
>>>>>>>>   };
>>>>>>>>   
>>>>>>>> +static const struct drm_plane_helper_funcs shmob_drm_primary_plane_helper_funcs = {
>>>>>>>> +	.atomic_check = shmob_drm_plane_atomic_check,
>>>>>>>> +	.atomic_update = shmob_drm_plane_atomic_update,
>>>>>>>> +	.atomic_disable = shmob_drm_plane_atomic_disable,
>>>>>>>> +	.get_scanout_buffer = drm_fb_dma_get_scanout_buffer,
>>>>>>>> +};
>>>>>>>> +
>>>>>>>>   static const struct drm_plane_funcs shmob_drm_plane_funcs = {
>>>>>>>>   	.update_plane = drm_atomic_helper_update_plane,
>>>>>>>>   	.disable_plane = drm_atomic_helper_disable_plane,
>>>>>>>> @@ -310,7 +317,12 @@ struct drm_plane *shmob_drm_plane_create(struct shmob_drm_device *sdev,
>>>>>>>>   
>>>>>>>>   	splane->index = index;
>>>>>>>>   
>>>>>>>> -	drm_plane_helper_add(&splane->base, &shmob_drm_plane_helper_funcs);
>>>>>>>> +	if (type == DRM_PLANE_TYPE_PRIMARY)
>>>>>>>> +		drm_plane_helper_add(&splane->base,
>>>>>>>> +				     &shmob_drm_primary_plane_helper_funcs);
>>>>>>>> +	else
>>>>>>>> +		drm_plane_helper_add(&splane->base,
>>>>>>>> +				     &shmob_drm_plane_helper_funcs);
>>>>>>>
>>>>>>> It's not very nice to have to provide different operations for the
>>>>>>> primary and overlay planes. The documentation of
>>>>>>> drm_fb_dma_get_scanout_buffer() states
>>>>>>>
>>>>>>>   * @plane: DRM primary plane
>>>>>>>
>>>>>>> If the intent is that only primary planes will be used to display the
>>>>>>> panic message, shouldn't drm_panic_register() skip overlay planes ? It
>>>>>>> would simplify drivers.
>>>>>>
>>>>>> What about the drivers where all the planes are actually universal?
>>>>>> In such a case the planes registered as primary can easily get replaced
>>>>>> by 'overlay' planes.
>>>>>
>>>>> Good point.
>>>>>
>>>>> Another option, if we wanted to avoid duplicating the drm_plane_funcs,
>>>>> would be to add a field to drm_plane to indicate whether the plane is
>>>>> suitable for drm_panic.
>>>>
>>>> ... or maybe let the driver decide. For the fully-universal-plane
>>>> devices we probably want to select the planes which cover the largest
>>>> part of the CRTC.
>>>
>>> Are there devices where certain planes can only cover a subset of the
>>> CRTC (apart from planes meant for cursors) ?
>>
>> On contemporary MSM devices any plane can cover any part of the screen,
>> including not having a plane that covers the full screen at all.
> 
> Ah, you meant picking the plane that is currently covering most of the
> screen. I thought you were talking about devices where some planes were
> restricted by the hardware to a subset of the CRTC.
> 
> I agree it would make sense to take both plane position and z-pos, as
> well as visibility and other related parameters, to pick the plane that
> is the most visible. Ideally this should be handled in drm_panic, not
> duplicated in drivers.

I'm not sure that drm_panic can figure out reliably on which plane it 
needs to draw. I think the driver has more information to take the right 
decision.
Also if you prefer, you can add the get_scanout_buffer() callback for 
all planes (to use the same helper fops), and then filter out in the 
callback for planes that are not suitable. I just find it cleaner to not 
register planes that the driver knows they will never be suitable (like 
cursor planes).

static void shmob_atomic_helper_get_scanout_buffer(struct drm_plane 
*plane, struct drm_scanout_buffer *sb))
{
	if (plane->type == DRM_PLANE_TYPE_PRIMARY)
		return drm_fb_dma_get_scanout_buffer(plane, sb);
	return -EOPNOTSUPP;
}

.get_scanout_buffer = shmob_atomic_helper_get_scanout_buffer,


-- 

Jocelyn

> 
>>> I think that what would matter the most in the end is selecting the
>>> plane that is on top of the stack, and that doesn't seem to be addressed
>>> by the drm_panic infrastructure. This is getting out of scope for this
>>> patch though :-)
>>>
>>>>> I don't think this patch should be blocked just for this reason, but I'm
>>>>> a bit bothered by duplicating the ops structure to indicate drm_panic
>>>>> support.
>>>>>
>>>>>>>>   
>>>>>>>>   	return &splane->base;
>>>>>>>>   }
>