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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, May 30, 2024 at 10:33 AM Dmitry Baryshkov
<dmitry.baryshkov@xxxxxxxxxx> wrote:
> On Thu, 30 May 2024 at 11:16, Jocelyn Falempe <jfalempe@xxxxxxxxxx> 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@xxxxxxxxx>
> > >>>>>>>> ---
> > >>>>>>>> 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?

Thank you!

[1] https://lore.kernel.org/r/b633568d2e3f405b21debdd60854fe39780254d6.1716816897.git.geert+renesas@xxxxxxxxx/
Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx

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





[Index of Archives]     [Linux Samsung SOC]     [Linux Wireless]     [Linux Kernel]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Device Mapper]

  Powered by Linux