[PATCH] drm/ast: Don't check new mode if CRTC is being disabled

[PATCH] drm/ast: Don't check new mode if CRTC is being disabled

[Thomas Zimmermann]

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@suse.de>
---
 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;
 	}
 
+	if (!state->enable)
+		return 0; /* no checks required if CRTC is being disabled */
+
 	ast_state = to_ast_crtc_state(state);
 
 	format = ast_state->format;
-- 
2.26.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/ast: Don't check new mode if CRTC is being disabled

[Sam Ravnborg]

Hi Thomas.

On Thu, Apr 30, 2020 at 11:13:30AM +0200, Thomas Zimmermann 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@suse.de>

Don't you miss:

Reported-by:
Tested-by:
Fixes:
???

	Sam

> ---
>  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;
>  	}
>  
> +	if (!state->enable)
> +		return 0; /* no checks required if CRTC is being disabled */
> +
>  	ast_state = to_ast_crtc_state(state);
>  
>  	format = ast_state->format;
> -- 
> 2.26.0
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/ast: Don't check new mode if CRTC is being disabled

[Thomas Zimmermann]

[-- Attachment #1.1.1: Type: text/plain, Size: 1508 bytes --]

Hi

Am 30.04.20 um 11:22 schrieb Sam Ravnborg:
> Hi Thomas.
> 
> On Thu, Apr 30, 2020 at 11:13:30AM +0200, Thomas Zimmermann 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@suse.de>
> 
> Don't you miss:
> 
> Reported-by:
> Tested-by:

Waiting for the reporter's ok.

> Fixes:

Indeed. :(

Best regards
Thomas

> ???
> 
> 	Sam
> 
>> ---
>>  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;
>>  	}
>>  
>> +	if (!state->enable)
>> +		return 0; /* no checks required if CRTC is being disabled */
>> +
>>  	ast_state = to_ast_crtc_state(state);
>>  
>>  	format = ast_state->format;
>> -- 
>> 2.26.0
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> 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 #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/ast: Don't check new mode if CRTC is being disabled

[Emil Velikov]

Hi Thomas,

Couple of fly-by ideas/suggestions.

On Thu, 30 Apr 2020 at 10:13, Thomas Zimmermann <tzimmermann@suse.de> 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@suse.de>
> ---
>  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 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?

HTH
-Emil
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/ast: Don't check new mode if CRTC is being disabled

[Thomas Zimmermann]

[-- Attachment #1.1.1: Type: text/plain, Size: 2889 bytes --]

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@suse.de> 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@suse.de>
>> ---
>>  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@lists.freedesktop.org
> 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 #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/ast: Don't check new mode if CRTC is being disabled

[Daniel Vetter]

On Mon, May 04, 2020 at 02:07:22PM +0200, Thomas Zimmermann wrote:
> 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@suse.de> 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@suse.de>
> >> ---
> >>  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.

Jumping out of ->atomic_check callbacks when stuff is all off is fairly
standard pattern. So much standard that I'm kinda wondering whether we
shouldn't just make it the default in atomic helpers - so many drivers
forget to do it and break in funny ways.
-Daniel

> 
> Best regards
> Thomas
> 
> > 
> > HTH
> > -Emil
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > 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
> 




> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel


-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/ast: Don't check new mode if CRTC is being disabled

[Emil Velikov]

On Mon, 4 May 2020 at 13:07, Thomas Zimmermann <tzimmermann@suse.de> wrote:
>
> 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@suse.de> 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@suse.de>
> >> ---
> >>  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.
>
No modeset support at all? Ouch.

Removing is one option a shorter/simpler one will be to expose zero connectors.
So any crazy^W brave soul can reinstate AST1180 support.

In either way - it's something for another day/series.

> >
> > 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.
>
Ack, makes sense.

Thanks
Emil
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/ast: Don't check new mode if CRTC is being disabled

[Thomas Zimmermann]

[-- Attachment #1.1.1: Type: text/plain, Size: 4128 bytes --]

Hi

Am 04.05.20 um 17:31 schrieb Daniel Vetter:
> On Mon, May 04, 2020 at 02:07:22PM +0200, Thomas Zimmermann wrote:
>> 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@suse.de> 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@suse.de>
>>>> ---
>>>>  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.
> 
> Jumping out of ->atomic_check callbacks when stuff is all off is fairly
> standard pattern. So much standard that I'm kinda wondering whether we
> shouldn't just make it the default in atomic helpers - so many drivers
> forget to do it and break in funny ways.

If there comes a driver that really must handle the 'disable case' in
its check handler, such a change would be hard to revert. Removing the
check from the DRM core could regress drivers that depend on the new
semantics.

Best regards
Thomas

> -Daniel
> 
>>
>> Best regards
>> Thomas
>>
>>>
>>> HTH
>>> -Emil
>>> _______________________________________________
>>> dri-devel mailing list
>>> dri-devel@lists.freedesktop.org
>>> 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
>>
> 
> 
> 
> 
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> 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 #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/ast: Don't check new mode if CRTC is being disabled

[Daniel Vetter]

On Wed, May 06, 2020 at 10:06:15AM +0200, Thomas Zimmermann wrote:
> Hi
> 
> Am 04.05.20 um 17:31 schrieb Daniel Vetter:
> > On Mon, May 04, 2020 at 02:07:22PM +0200, Thomas Zimmermann wrote:
> >> 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@suse.de> 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@suse.de>
> >>>> ---
> >>>>  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.
> > 
> > Jumping out of ->atomic_check callbacks when stuff is all off is fairly
> > standard pattern. So much standard that I'm kinda wondering whether we
> > shouldn't just make it the default in atomic helpers - so many drivers
> > forget to do it and break in funny ways.
> 
> If there comes a driver that really must handle the 'disable case' in
> its check handler, such a change would be hard to revert. Removing the
> check from the DRM core could regress drivers that depend on the new
> semantics.

Yeah I know, that's why I don't want to push this into helpers (it
wouldn't ever be in core code). But it surely is tempting, because we have
tons of bugs in this area. I've made a patch series a while ago that only
cleaned up some of the minor confusion between plane_state->crtc and
plane_state->fb (they're only set together or neither). Losing choice
either way :-/
-Daniel

> 
> Best regards
> Thomas
> 
> > -Daniel
> > 
> >>
> >> Best regards
> >> Thomas
> >>
> >>>
> >>> HTH
> >>> -Emil
> >>> _______________________________________________
> >>> dri-devel mailing list
> >>> dri-devel@lists.freedesktop.org
> >>> 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
> >>
> > 
> > 
> > 
> > 
> >> _______________________________________________
> >> dri-devel mailing list
> >> dri-devel@lists.freedesktop.org
> >> 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
> 




-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel