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:
> >>>>> 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@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.
> 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;
> >>>>>>>> }
> >
>
--
With best wishes
Dmitry
