[PATCH] drm: renesas: shmobile: Add drm_panic support
Jocelyn Falempe
jfalempe at redhat.com
Tue Sep 3 13:23:57 UTC 2024
On 29/08/2024 15:53, Geert Uytterhoeven wrote:
> On Thu, May 30, 2024 at 10:33 AM Dmitry Baryshkov
> <dmitry.baryshkov at linaro.org> wrote:
>> On Thu, 30 May 2024 at 11:16, Jocelyn Falempe <jfalempe at redhat.com> wrote:
>>> 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:
>>>>>>>> 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.
>>
>> I think there should be a default implementation which fits 80% of the
>> cases (single fixed PRIMARY plane per output) but if the driver needs
>> it, it should be able to override the behaviour.
>
> Did anything like this materialize, or is this patch (and its rcar-du
> counterpart [1]) good to apply as-is?
No I didn't find a better option yet, and I think it is good as-is.
If duplicating the helper funcs is really a problem, I think the less
intrusive option, would be to add a "bool panic_allow_overlay_plane" to
struct drm_mode_config, and use that in drm_panic_register(), to
register only primary planes if it's not set ?
So drivers that want to draw the panic on overlay plane can opt-in.
Best regards,
--
Jocelyn
>
> Thank you!
>
> [1] https://lore.kernel.org/r/b633568d2e3f405b21debdd60854fe39780254d6.1716816897.git.geert+renesas@glider.be/
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
> -- Linus Torvalds
>
More information about the dri-devel
mailing list