On Thu, 30 May 2024 at 11:16, Jocelyn Falempe <jfale...@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: > >>>>> 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+rene...@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. > 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