[PATCH] drm/amd/display: check plane state before validating fbc

[PATCH] drm/amd/display: check plane state before validating fbc

[S, Shirish]

From: Shirish S <shirish.s@amd.com>

While validation fbc, array_mode of the pipe is accessed without checking plane_state exists for it.
Causing to null pointer dereferencing followed by reboot when a crtc associated with external display(not
connected) is page flipped.

This patch adds a check for plane_state before using it to validate fbc.

Signed-off-by: Shirish S <shirish.s@amd.com>
Reviewed-by: Roman Li <Roman.Li@amd.com>
---
 drivers/gpu/drm/amd/display/dc/dce110/dce110_hw_sequencer.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/amd/display/dc/dce110/dce110_hw_sequencer.c b/drivers/gpu/drm/amd/display/dc/dce110/dce110_hw_sequencer.c
index ee3b944..a6cd63a 100644
--- a/drivers/gpu/drm/amd/display/dc/dce110/dce110_hw_sequencer.c
+++ b/drivers/gpu/drm/amd/display/dc/dce110/dce110_hw_sequencer.c
@@ -1724,6 +1724,10 @@ static enum dc_status validate_fbc(struct dc *dc,
 	if (pipe_ctx->stream->sink->link->psr_enabled)
 		return DC_ERROR_UNEXPECTED;
 
+	/* Nothing to compress */
+	if (!pipe_ctx->plane_state)
+		return DC_ERROR_UNEXPECTED;
+
 	/* Only for non-linear tiling */
 	if (pipe_ctx->plane_state->tiling_info.gfx8.array_mode == DC_ARRAY_LINEAR_GENERAL)
 		return DC_ERROR_UNEXPECTED;
--
2.7.4

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amd/display: check plane state before validating fbc

[Harry Wentland]

On 2017-11-22 02:13 AM, S, Shirish wrote:
> From: Shirish S <shirish.s@amd.com>
> 
> While validation fbc, array_mode of the pipe is accessed without checking plane_state exists for it.
> Causing to null pointer dereferencing followed by reboot when a crtc associated with external display(not
> connected) is page flipped.
> 
> This patch adds a check for plane_state before using it to validate fbc.
> 
> Signed-off-by: Shirish S <shirish.s@amd.com>
> Reviewed-by: Roman Li <Roman.Li@amd.com>

Reviewed-by: Harry Wentland <harry.wentland@amd.com>

Harry

> ---
>  drivers/gpu/drm/amd/display/dc/dce110/dce110_hw_sequencer.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/gpu/drm/amd/display/dc/dce110/dce110_hw_sequencer.c b/drivers/gpu/drm/amd/display/dc/dce110/dce110_hw_sequencer.c
> index ee3b944..a6cd63a 100644
> --- a/drivers/gpu/drm/amd/display/dc/dce110/dce110_hw_sequencer.c
> +++ b/drivers/gpu/drm/amd/display/dc/dce110/dce110_hw_sequencer.c
> @@ -1724,6 +1724,10 @@ static enum dc_status validate_fbc(struct dc *dc,
>  	if (pipe_ctx->stream->sink->link->psr_enabled)
>  		return DC_ERROR_UNEXPECTED;
>  
> +	/* Nothing to compress */
> +	if (!pipe_ctx->plane_state)
> +		return DC_ERROR_UNEXPECTED;
> +
>  	/* Only for non-linear tiling */
>  	if (pipe_ctx->plane_state->tiling_info.gfx8.array_mode == DC_ARRAY_LINEAR_GENERAL)
>  		return DC_ERROR_UNEXPECTED;
> --
> 2.7.4
> 
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
> 
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amd/display: check plane state before validating fbc

[Andrey Grodzovsky]

The patch is good, but why validate_fbc is being called from 
dce110_apply_ctx_to_hw (atomic_commit) ? It should be called from 
atomic_check.

Thanks,
Andrey

On 2017-11-22 02:13 AM, S, Shirish wrote:
> From: Shirish S <shirish.s@amd.com>
>
> While validation fbc, array_mode of the pipe is accessed without checking plane_state exists for it.
> Causing to null pointer dereferencing followed by reboot when a crtc associated with external display(not
> connected) is page flipped.
>
> This patch adds a check for plane_state before using it to validate fbc.
>
> Signed-off-by: Shirish S <shirish.s@amd.com>
> Reviewed-by: Roman Li <Roman.Li@amd.com>
> ---
>   drivers/gpu/drm/amd/display/dc/dce110/dce110_hw_sequencer.c | 4 ++++
>   1 file changed, 4 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/display/dc/dce110/dce110_hw_sequencer.c b/drivers/gpu/drm/amd/display/dc/dce110/dce110_hw_sequencer.c
> index ee3b944..a6cd63a 100644
> --- a/drivers/gpu/drm/amd/display/dc/dce110/dce110_hw_sequencer.c
> +++ b/drivers/gpu/drm/amd/display/dc/dce110/dce110_hw_sequencer.c
> @@ -1724,6 +1724,10 @@ static enum dc_status validate_fbc(struct dc *dc,
>   	if (pipe_ctx->stream->sink->link->psr_enabled)
>   		return DC_ERROR_UNEXPECTED;
>   
> +	/* Nothing to compress */
> +	if (!pipe_ctx->plane_state)
> +		return DC_ERROR_UNEXPECTED;
> +
>   	/* Only for non-linear tiling */
>   	if (pipe_ctx->plane_state->tiling_info.gfx8.array_mode == DC_ARRAY_LINEAR_GENERAL)
>   		return DC_ERROR_UNEXPECTED;
> --
> 2.7.4
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amd/display: check plane state before validating fbc

[Harry Wentland]

On 2017-11-24 05:07 PM, Andrey Grodzovsky wrote:
> The patch is good, but why validate_fbc is being called from dce110_apply_ctx_to_hw (atomic_commit) ? It should be called from atomic_check.
> 

Ideally yes, but I don't think this has any impact on bandwidth and mode validation, i.e. we should never fail to apply an atomic state, no matter whether we do FBC. If we let user-mode decide whether to do FBC or not it would need to be a property and check in atomic_check but I don't think it makes any sense to expose this outside DC.

Harry

> Thanks,
> Andrey
> 
> On 2017-11-22 02:13 AM, S, Shirish wrote:
>> From: Shirish S <shirish.s@amd.com>
>>
>> While validation fbc, array_mode of the pipe is accessed without checking plane_state exists for it.
>> Causing to null pointer dereferencing followed by reboot when a crtc associated with external display(not
>> connected) is page flipped.
>>
>> This patch adds a check for plane_state before using it to validate fbc.
>>
>> Signed-off-by: Shirish S <shirish.s@amd.com>
>> Reviewed-by: Roman Li <Roman.Li@amd.com>
>> ---
>>   drivers/gpu/drm/amd/display/dc/dce110/dce110_hw_sequencer.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/amd/display/dc/dce110/dce110_hw_sequencer.c b/drivers/gpu/drm/amd/display/dc/dce110/dce110_hw_sequencer.c
>> index ee3b944..a6cd63a 100644
>> --- a/drivers/gpu/drm/amd/display/dc/dce110/dce110_hw_sequencer.c
>> +++ b/drivers/gpu/drm/amd/display/dc/dce110/dce110_hw_sequencer.c
>> @@ -1724,6 +1724,10 @@ static enum dc_status validate_fbc(struct dc *dc,
>>       if (pipe_ctx->stream->sink->link->psr_enabled)
>>           return DC_ERROR_UNEXPECTED;
>>   +    /* Nothing to compress */
>> +    if (!pipe_ctx->plane_state)
>> +        return DC_ERROR_UNEXPECTED;
>> +
>>       /* Only for non-linear tiling */
>>       if (pipe_ctx->plane_state->tiling_info.gfx8.array_mode == DC_ARRAY_LINEAR_GENERAL)
>>           return DC_ERROR_UNEXPECTED;
>> -- 
>> 2.7.4
>>
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
> 
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amd/display: check plane state before validating fbc

[Andrey Grodzovsky]

On 11/26/2017 11:57 AM, Harry Wentland wrote:
> On 2017-11-24 05:07 PM, Andrey Grodzovsky wrote:
>> The patch is good, but why validate_fbc is being called from dce110_apply_ctx_to_hw (atomic_commit) ? It should be called from atomic_check.
>>
> Ideally yes, but I don't think this has any impact on bandwidth and mode validation, i.e. we should never fail to apply an atomic state, no matter whether we do FBC. If we let user-mode decide whether to do FBC or not it would need to be a property and check in atomic_check but I don't think it makes any sense to expose this outside DC.
>
> Harry

I see, as long as enable_fbc return code is ignored as it is today that ok.

Thanks,
Andrey

>
>> Thanks,
>> Andrey
>>
>> On 2017-11-22 02:13 AM, S, Shirish wrote:
>>> From: Shirish S <shirish.s@amd.com>
>>>
>>> While validation fbc, array_mode of the pipe is accessed without checking plane_state exists for it.
>>> Causing to null pointer dereferencing followed by reboot when a crtc associated with external display(not
>>> connected) is page flipped.
>>>
>>> This patch adds a check for plane_state before using it to validate fbc.
>>>
>>> Signed-off-by: Shirish S <shirish.s@amd.com>
>>> Reviewed-by: Roman Li <Roman.Li@amd.com>
>>> ---
>>>    drivers/gpu/drm/amd/display/dc/dce110/dce110_hw_sequencer.c | 4 ++++
>>>    1 file changed, 4 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/amd/display/dc/dce110/dce110_hw_sequencer.c b/drivers/gpu/drm/amd/display/dc/dce110/dce110_hw_sequencer.c
>>> index ee3b944..a6cd63a 100644
>>> --- a/drivers/gpu/drm/amd/display/dc/dce110/dce110_hw_sequencer.c
>>> +++ b/drivers/gpu/drm/amd/display/dc/dce110/dce110_hw_sequencer.c
>>> @@ -1724,6 +1724,10 @@ static enum dc_status validate_fbc(struct dc *dc,
>>>        if (pipe_ctx->stream->sink->link->psr_enabled)
>>>            return DC_ERROR_UNEXPECTED;
>>>    +    /* Nothing to compress */
>>> +    if (!pipe_ctx->plane_state)
>>> +        return DC_ERROR_UNEXPECTED;
>>> +
>>>        /* Only for non-linear tiling */
>>>        if (pipe_ctx->plane_state->tiling_info.gfx8.array_mode == DC_ARRAY_LINEAR_GENERAL)
>>>            return DC_ERROR_UNEXPECTED;
>>> -- 
>>> 2.7.4
>>>
>>> _______________________________________________
>>> amd-gfx mailing list
>>> amd-gfx@lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amd/display: check plane state before validating fbc

[Harry Wentland]

On 2017-11-27 07:38 AM, Andrey Grodzovsky wrote:
> 
> 
> On 11/26/2017 11:57 AM, Harry Wentland wrote:
>> On 2017-11-24 05:07 PM, Andrey Grodzovsky wrote:
>>> The patch is good, but why validate_fbc is being called from dce110_apply_ctx_to_hw (atomic_commit) ? It should be called from atomic_check.
>>>
>> Ideally yes, but I don't think this has any impact on bandwidth and mode validation, i.e. we should never fail to apply an atomic state, no matter whether we do FBC. If we let user-mode decide whether to do FBC or not it would need to be a property and check in atomic_check but I don't think it makes any sense to expose this outside DC.
>>
>> Harry
> 
> I see, as long as enable_fbc return code is ignored as it is today that ok.
> 

Maybe we just shouldn't return an error code here. dce110_apply_ctx_to_hw probably shouldn't return one either then, and apply_single_controller_ctx_to_hw_plane (just a couple lines before enable_fbc) shouldn't fail. I might have to look at those sometime.

Harry

> Thanks,
> Andrey
> 
>>
>>> Thanks,
>>> Andrey
>>>
>>> On 2017-11-22 02:13 AM, S, Shirish wrote:
>>>> From: Shirish S <shirish.s@amd.com>
>>>>
>>>> While validation fbc, array_mode of the pipe is accessed without checking plane_state exists for it.
>>>> Causing to null pointer dereferencing followed by reboot when a crtc associated with external display(not
>>>> connected) is page flipped.
>>>>
>>>> This patch adds a check for plane_state before using it to validate fbc.
>>>>
>>>> Signed-off-by: Shirish S <shirish.s@amd.com>
>>>> Reviewed-by: Roman Li <Roman.Li@amd.com>
>>>> ---
>>>>    drivers/gpu/drm/amd/display/dc/dce110/dce110_hw_sequencer.c | 4 ++++
>>>>    1 file changed, 4 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/display/dc/dce110/dce110_hw_sequencer.c b/drivers/gpu/drm/amd/display/dc/dce110/dce110_hw_sequencer.c
>>>> index ee3b944..a6cd63a 100644
>>>> --- a/drivers/gpu/drm/amd/display/dc/dce110/dce110_hw_sequencer.c
>>>> +++ b/drivers/gpu/drm/amd/display/dc/dce110/dce110_hw_sequencer.c
>>>> @@ -1724,6 +1724,10 @@ static enum dc_status validate_fbc(struct dc *dc,
>>>>        if (pipe_ctx->stream->sink->link->psr_enabled)
>>>>            return DC_ERROR_UNEXPECTED;
>>>>    +    /* Nothing to compress */
>>>> +    if (!pipe_ctx->plane_state)
>>>> +        return DC_ERROR_UNEXPECTED;
>>>> +
>>>>        /* Only for non-linear tiling */
>>>>        if (pipe_ctx->plane_state->tiling_info.gfx8.array_mode == DC_ARRAY_LINEAR_GENERAL)
>>>>            return DC_ERROR_UNEXPECTED;
>>>> -- 
>>>> 2.7.4
>>>>
>>>> _______________________________________________
>>>> amd-gfx mailing list
>>>> amd-gfx@lists.freedesktop.org
>>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
> 
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amd/display: check plane state before validating fbc

[Andrey Grodzovsky]

On 11/27/2017 09:28 AM, Harry Wentland wrote:
>
> On 2017-11-27 07:38 AM, Andrey Grodzovsky wrote:
>>
>> On 11/26/2017 11:57 AM, Harry Wentland wrote:
>>> On 2017-11-24 05:07 PM, Andrey Grodzovsky wrote:
>>>> The patch is good, but why validate_fbc is being called from dce110_apply_ctx_to_hw (atomic_commit) ? It should be called from atomic_check.
>>>>
>>> Ideally yes, but I don't think this has any impact on bandwidth and mode validation, i.e. we should never fail to apply an atomic state, no matter whether we do FBC. If we let user-mode decide whether to do FBC or not it would need to be a property and check in atomic_check but I don't think it makes any sense to expose this outside DC.
>>>
>>> Harry
>> I see, as long as enable_fbc return code is ignored as it is today that ok.
>>
> Maybe we just shouldn't return an error code here. dce110_apply_ctx_to_hw probably shouldn't return one either then, and apply_single_controller_ctx_to_hw_plane (just a couple lines before enable_fbc) shouldn't fail. I might have to look at those sometime.
>
> Harry

Well, it seems wrong to have a validation function which never fails or 
it's return value ignored, maybe just merge validate_fbc into enable_fbc 
, drop all the error codes and rename enable_fbc into try_enable_fbc

Thanks,
Andrey

>
>> Thanks,
>> Andrey
>>
>>>> Thanks,
>>>> Andrey
>>>>
>>>> On 2017-11-22 02:13 AM, S, Shirish wrote:
>>>>> From: Shirish S <shirish.s@amd.com>
>>>>>
>>>>> While validation fbc, array_mode of the pipe is accessed without checking plane_state exists for it.
>>>>> Causing to null pointer dereferencing followed by reboot when a crtc associated with external display(not
>>>>> connected) is page flipped.
>>>>>
>>>>> This patch adds a check for plane_state before using it to validate fbc.
>>>>>
>>>>> Signed-off-by: Shirish S <shirish.s@amd.com>
>>>>> Reviewed-by: Roman Li <Roman.Li@amd.com>
>>>>> ---
>>>>>     drivers/gpu/drm/amd/display/dc/dce110/dce110_hw_sequencer.c | 4 ++++
>>>>>     1 file changed, 4 insertions(+)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/amd/display/dc/dce110/dce110_hw_sequencer.c b/drivers/gpu/drm/amd/display/dc/dce110/dce110_hw_sequencer.c
>>>>> index ee3b944..a6cd63a 100644
>>>>> --- a/drivers/gpu/drm/amd/display/dc/dce110/dce110_hw_sequencer.c
>>>>> +++ b/drivers/gpu/drm/amd/display/dc/dce110/dce110_hw_sequencer.c
>>>>> @@ -1724,6 +1724,10 @@ static enum dc_status validate_fbc(struct dc *dc,
>>>>>         if (pipe_ctx->stream->sink->link->psr_enabled)
>>>>>             return DC_ERROR_UNEXPECTED;
>>>>>     +    /* Nothing to compress */
>>>>> +    if (!pipe_ctx->plane_state)
>>>>> +        return DC_ERROR_UNEXPECTED;
>>>>> +
>>>>>         /* Only for non-linear tiling */
>>>>>         if (pipe_ctx->plane_state->tiling_info.gfx8.array_mode == DC_ARRAY_LINEAR_GENERAL)
>>>>>             return DC_ERROR_UNEXPECTED;
>>>>> -- 
>>>>> 2.7.4
>>>>>
>>>>> _______________________________________________
>>>>> amd-gfx mailing list
>>>>> amd-gfx@lists.freedesktop.org
>>>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amd/display: check plane state before validating fbc

[Harry Wentland]

On 2017-11-27 09:37 AM, Andrey Grodzovsky wrote:
> 
> 
> On 11/27/2017 09:28 AM, Harry Wentland wrote:
>>
>> On 2017-11-27 07:38 AM, Andrey Grodzovsky wrote:
>>>
>>> On 11/26/2017 11:57 AM, Harry Wentland wrote:
>>>> On 2017-11-24 05:07 PM, Andrey Grodzovsky wrote:
>>>>> The patch is good, but why validate_fbc is being called from dce110_apply_ctx_to_hw (atomic_commit) ? It should be called from atomic_check.
>>>>>
>>>> Ideally yes, but I don't think this has any impact on bandwidth and mode validation, i.e. we should never fail to apply an atomic state, no matter whether we do FBC. If we let user-mode decide whether to do FBC or not it would need to be a property and check in atomic_check but I don't think it makes any sense to expose this outside DC.
>>>>
>>>> Harry
>>> I see, as long as enable_fbc return code is ignored as it is today that ok.
>>>
>> Maybe we just shouldn't return an error code here. dce110_apply_ctx_to_hw probably shouldn't return one either then, and apply_single_controller_ctx_to_hw_plane (just a couple lines before enable_fbc) shouldn't fail. I might have to look at those sometime.
>>
>> Harry
> 
> Well, it seems wrong to have a validation function which never fails or it's return value ignored, maybe just merge validate_fbc into enable_fbc , drop all the error codes and rename enable_fbc into try_enable_fbc

This function just seems to mislabeled. decide_fbc might be better. It's only called from enable_fbc to decide whether or not to do FBC.

Roma, would it make sense to rename this function to avoid confusion?

Harry

> 
> Thanks,
> Andrey
> 
>>
>>> Thanks,
>>> Andrey
>>>
>>>>> Thanks,
>>>>> Andrey
>>>>>
>>>>> On 2017-11-22 02:13 AM, S, Shirish wrote:
>>>>>> From: Shirish S <shirish.s@amd.com>
>>>>>>
>>>>>> While validation fbc, array_mode of the pipe is accessed without checking plane_state exists for it.
>>>>>> Causing to null pointer dereferencing followed by reboot when a crtc associated with external display(not
>>>>>> connected) is page flipped.
>>>>>>
>>>>>> This patch adds a check for plane_state before using it to validate fbc.
>>>>>>
>>>>>> Signed-off-by: Shirish S <shirish.s@amd.com>
>>>>>> Reviewed-by: Roman Li <Roman.Li@amd.com>
>>>>>> ---
>>>>>>     drivers/gpu/drm/amd/display/dc/dce110/dce110_hw_sequencer.c | 4 ++++
>>>>>>     1 file changed, 4 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/amd/display/dc/dce110/dce110_hw_sequencer.c b/drivers/gpu/drm/amd/display/dc/dce110/dce110_hw_sequencer.c
>>>>>> index ee3b944..a6cd63a 100644
>>>>>> --- a/drivers/gpu/drm/amd/display/dc/dce110/dce110_hw_sequencer.c
>>>>>> +++ b/drivers/gpu/drm/amd/display/dc/dce110/dce110_hw_sequencer.c
>>>>>> @@ -1724,6 +1724,10 @@ static enum dc_status validate_fbc(struct dc *dc,
>>>>>>         if (pipe_ctx->stream->sink->link->psr_enabled)
>>>>>>             return DC_ERROR_UNEXPECTED;
>>>>>>     +    /* Nothing to compress */
>>>>>> +    if (!pipe_ctx->plane_state)
>>>>>> +        return DC_ERROR_UNEXPECTED;
>>>>>> +
>>>>>>         /* Only for non-linear tiling */
>>>>>>         if (pipe_ctx->plane_state->tiling_info.gfx8.array_mode == DC_ARRAY_LINEAR_GENERAL)
>>>>>>             return DC_ERROR_UNEXPECTED;
>>>>>> -- 
>>>>>> 2.7.4
>>>>>>
>>>>>> _______________________________________________
>>>>>> amd-gfx mailing list
>>>>>> amd-gfx@lists.freedesktop.org
>>>>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
> 
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx