On Thu, May 24, 2018 at 05:42:02AM +0300, Haneen Mohammed wrote: > On Wed, May 23, 2018 at 11:15:18AM +0200, Daniel Vetter wrote: > > On Mon, May 21, 2018 at 10:04:23PM -0300, Rodrigo Siqueira wrote: > > > This series of patches add a centralized initialization mechanism, a > > > single CRTC with a plane, an encoder, and extra module information. > > > > > > Changes in v2: > > > - Remove unused definitions > > > - Improve file names > > > - Improve code separation > > > - Remove unnecessary formats > > > > Oops, I merged v2 already. I think we need follow-up patches. > > > > > Changes in v3: > > > - Adds drm_crtc_helper_funcs with a simple atomic_check > > > > Sorry for the late comment on v2, but I think we need to figure out why > > this goes boom first. It imo shouldn't. > > > > > - Adds extra hooks for drm_connector_funcs hooks (reset, > > > atomic_duplicate_state, atomic_destroy_state) > > > > Hm, reset shouldn't be required. Why do you need it? > > > > I've checked the code, and I think the memory for [plane/connector/crtc] > states that are used later in duplicate/destroy are allocated in drm_atomic_helper_*_reset. > Otherwise checking /sys/kernel/debug/dri/0/state or running the igt > tests causes null pointer dereference error. Yes, but drm_mode_config_reset is a helper, and not mandatory. Most drivers use that to initialize the software state structures, but e.g. i915 reads out the actual hardware state, and uses that to reconstruct the software state. This allows for smooth take over of the boot loader screen state, avoiding flickering. That's why the ->reset hooks aren't mandatory, but duplicate/destroy hooks are. > > > Wrt duplicate/destroy state, those are mandatory. I think it'd be good to > > have checks for those in the drm_*_init functions, but only for atomic > > drivers. You can use drm_drv_uses_atomic_modeset() and WARN_ON(). There's > > a bunch of examples already for checking for this stuff, see e.g. > > dma_fence_init(). > > > > > - Adds drm_connector_helper_funcs > > > - Adds drm_plane_helper_funcs > > > > Same here, would be good to add WARN_ON to the relevant _init() functions > > to make sure all the mandatory stuff is there to begin with. > > > > Since Rodrigo has typed the fixes to vkms already, could you Haneen look > > into adding these checks to the core drm core? > > > > Sure, I'll work on that. Cool. When you do that, pls also double-check that the kerneldoc comments are accurate and up-to-date. Thanks, Daniel > > > Thanks, Daniel > > > > > - Changes in the commit messages > > > > > > Rodrigo Siqueira (3): > > > drm/vkms: Add mode_config initialization > > > drm/vkms: Add basic CRTC initialization > > > drm/vkms: Add extra information about vkms > > > > > > drivers/gpu/drm/Kconfig | 8 +- > > > drivers/gpu/drm/vkms/Makefile | 2 +- > > > drivers/gpu/drm/vkms/vkms_crtc.c | 47 ++++++++++++ > > > drivers/gpu/drm/vkms/vkms_drv.c | 55 +++++--------- > > > drivers/gpu/drm/vkms/vkms_drv.h | 33 +++++++- > > > drivers/gpu/drm/vkms/vkms_output.c | 118 +++++++++++++++++++++++++++++ > > > drivers/gpu/drm/vkms/vkms_plane.c | 62 +++++++++++++++ > > > 7 files changed, 285 insertions(+), 40 deletions(-) > > > create mode 100644 drivers/gpu/drm/vkms/vkms_crtc.c > > > create mode 100644 drivers/gpu/drm/vkms/vkms_output.c > > > create mode 100644 drivers/gpu/drm/vkms/vkms_plane.c > > > > > > -- > > > 2.17.0 > > > > > > > -- > > Daniel Vetter > > Software Engineer, Intel Corporation > > http://blog.ffwll.ch -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
