Hi Emil Am 01.05.20 um 15:20 schrieb Emil Velikov: > Hi Thomas, > > Couple of fly-by ideas/suggestions. > > On Thu, 30 Apr 2020 at 10:13, Thomas Zimmermann <tzimmermann@xxxxxxx> wrote: >> >> Suspending failed because there's no mode if the CRTC is being >> disabled. Early-out in this case. This fixes runtime PM for ast. >> >> Signed-off-by: Thomas Zimmermann <tzimmermann@xxxxxxx> >> --- >> drivers/gpu/drm/ast/ast_mode.c | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c >> index 7a9f20a2fd303..089b7d9a0cf3f 100644 >> --- a/drivers/gpu/drm/ast/ast_mode.c >> +++ b/drivers/gpu/drm/ast/ast_mode.c >> @@ -801,6 +801,9 @@ static int ast_crtc_helper_atomic_check(struct drm_crtc *crtc, >> return -EINVAL; > Unrelated: > This feels quite dirty. If AST1180 does not support atomic modeset > simply remove the DRIVER_ATOMIC bit. > You can do that at runtime, via drm_device::driver_features in say, > ast_detect_chip()? The line you commented on dates back to non-atomic modesetting, but I don't know what the story behind AST1180 is. It is explicitly disabled in the list of PCI IDs, but the driver has plenty of code for it. It looks as if the chip can only do pageflipping with a pre-set video mode. As it is right now, the AST1180 code could probably be deleted entirely. > > The drm_driver::driver_features is immutable, or it ought to be. > >> } >> >> + if (!state->enable) >> + return 0; /* no checks required if CRTC is being disabled */ >> + > I cannot think of a reason why a driver would need to perform > crtc_atomic_check, if the crtc is being disabled. > Can you spot any? If not, this should be better served in core, which > calls this callback. > Correct? Ast is a bit of a special case, because it tests the incoming mode against a list of re-defined modes. With the crtc being disabled, the incoming mode is 0 in all fields. Obviously that's not a valid mode, and we need that additional test here. In the general case, I'd see 'crtc check' as part of the larger atomic infrastructure. I can imagine that configurations require the CRTC to be enabled before other HW blocks work. So a driver might have a reason to run crtc's check even for disabled crtcs (at least to verify that the crtc is not disabled). I don't think this can be handled in the core easily. Best regards Thomas > > HTH > -Emil > _______________________________________________ > dri-devel mailing list > dri-devel@xxxxxxxxxxxxxxxxxxxxx > https://lists.freedesktop.org/mailman/listinfo/dri-devel > -- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Felix Imendörffer
Attachment:
signature.asc
Description: OpenPGP digital signature
_______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
