[v3,1/9] drm/mediatek: dp: Cache EDID for eDP panel

  Since eDP panels are not removable it is safe to cache the EDID:
this will avoid a relatively long read transaction at every PM
resume that is unnecessary only in the "special" case of eDP,
hence speeding it up a little, as from now on, as resume operation,
we will perform only link training.

Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 drivers/gpu/drm/mediatek/mtk_dp.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)
  

On 04/04/2023 12:47, AngeloGioacchino Del Regno wrote:
> Since eDP panels are not removable it is safe to cache the EDID:
> this will avoid a relatively long read transaction at every PM
> resume that is unnecessary only in the "special" case of eDP,
> hence speeding it up a little, as from now on, as resume operation,
> we will perform only link training.
> 
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> ---
>   drivers/gpu/drm/mediatek/mtk_dp.c | 11 ++++++++++-
>   1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/mediatek/mtk_dp.c b/drivers/gpu/drm/mediatek/mtk_dp.c
> index 1f94fcc144d3..84f82cc68672 100644
> --- a/drivers/gpu/drm/mediatek/mtk_dp.c
> +++ b/drivers/gpu/drm/mediatek/mtk_dp.c
> @@ -118,6 +118,7 @@ struct mtk_dp {
>   	const struct mtk_dp_data *data;
>   	struct mtk_dp_info info;
>   	struct mtk_dp_train_info train_info;
> +	struct edid *edid;
>   
>   	struct platform_device *phy_dev;
>   	struct phy *phy;
> @@ -1993,7 +1994,11 @@ static struct edid *mtk_dp_get_edid(struct drm_bridge *bridge,
>   		usleep_range(2000, 5000);
>   	}
>   
> -	new_edid = drm_get_edid(connector, &mtk_dp->aux.ddc);
> +	/* eDP panels aren't removable, so we can return a cached EDID. */
> +	if (mtk_dp->edid && mtk_dp->bridge.type == DRM_MODE_CONNECTOR_eDP)
> +		new_edid = drm_edid_duplicate(mtk_dp->edid);
> +	else
> +		new_edid = drm_get_edid(connector, &mtk_dp->aux.ddc);

Maybe it would make sense to add a macro for the check of mtk_dp->bridge.type == 
DRM_MODE_CONNECTOR_eDP
it would make the code more readable.

>   
>   	/*
>   	 * Parse capability here to let atomic_get_input_bus_fmts and
> @@ -2022,6 +2027,10 @@ static struct edid *mtk_dp_get_edid(struct drm_bridge *bridge,
>   		drm_atomic_bridge_chain_post_disable(bridge, connector->state->state);
>   	}
>   
> +	/* If this is an eDP panel and the read EDID is good, cache it for later */
> +	if (mtk_dp->bridge.type == DRM_MODE_CONNECTOR_eDP && !mtk_dp->edid && new_edid)
> +		mtk_dp->edid = drm_edid_duplicate(new_edid);
> +

How about putting this in an else if branch of mtk_dp_parse_capabilities. At 
least we could get rid of the check regarding if new_edid != NULL.

I was thinking on how to put both if statements in one block, but I think the 
problem is, that we would leak memory if the capability parsing failes due to 
the call to drm_edid_duplicate(). Correct?

Regards,
Matthais

>   	return new_edid;
>   }.	/
>
  

Il 12/04/23 09:08, Matthias Brugger ha scritto:
> 
> 
> On 04/04/2023 12:47, AngeloGioacchino Del Regno wrote:
>> Since eDP panels are not removable it is safe to cache the EDID:
>> this will avoid a relatively long read transaction at every PM
>> resume that is unnecessary only in the "special" case of eDP,
>> hence speeding it up a little, as from now on, as resume operation,
>> we will perform only link training.
>>
>> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
>> ---
>>   drivers/gpu/drm/mediatek/mtk_dp.c | 11 ++++++++++-
>>   1 file changed, 10 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/mediatek/mtk_dp.c b/drivers/gpu/drm/mediatek/mtk_dp.c
>> index 1f94fcc144d3..84f82cc68672 100644
>> --- a/drivers/gpu/drm/mediatek/mtk_dp.c
>> +++ b/drivers/gpu/drm/mediatek/mtk_dp.c
>> @@ -118,6 +118,7 @@ struct mtk_dp {
>>       const struct mtk_dp_data *data;
>>       struct mtk_dp_info info;
>>       struct mtk_dp_train_info train_info;
>> +    struct edid *edid;
>>       struct platform_device *phy_dev;
>>       struct phy *phy;
>> @@ -1993,7 +1994,11 @@ static struct edid *mtk_dp_get_edid(struct drm_bridge 
>> *bridge,
>>           usleep_range(2000, 5000);
>>       }
>> -    new_edid = drm_get_edid(connector, &mtk_dp->aux.ddc);
>> +    /* eDP panels aren't removable, so we can return a cached EDID. */
>> +    if (mtk_dp->edid && mtk_dp->bridge.type == DRM_MODE_CONNECTOR_eDP)
>> +        new_edid = drm_edid_duplicate(mtk_dp->edid);
>> +    else
>> +        new_edid = drm_get_edid(connector, &mtk_dp->aux.ddc);
> 
> Maybe it would make sense to add a macro for the check of mtk_dp->bridge.type == 
> DRM_MODE_CONNECTOR_eDP
> it would make the code more readable.
> 

I had the same idea... but then avoided that because in most (if not all?) of the
DRM drivers (at least, the one I've read) this check is always open coded, so I
wrote it like that for consistency and nothing else.

I have no strong opinions on that though!

>>       /*
>>        * Parse capability here to let atomic_get_input_bus_fmts and
>> @@ -2022,6 +2027,10 @@ static struct edid *mtk_dp_get_edid(struct drm_bridge 
>> *bridge,
>>           drm_atomic_bridge_chain_post_disable(bridge, connector->state->state);
>>       }
>> +    /* If this is an eDP panel and the read EDID is good, cache it for later */
>> +    if (mtk_dp->bridge.type == DRM_MODE_CONNECTOR_eDP && !mtk_dp->edid && new_edid)
>> +        mtk_dp->edid = drm_edid_duplicate(new_edid);
>> +
> 
> How about putting this in an else if branch of mtk_dp_parse_capabilities. At least 
> we could get rid of the check regarding if new_edid != NULL.
> 
> I was thinking on how to put both if statements in one block, but I think the 
> problem is, that we would leak memory if the capability parsing failes due to the 
> call to drm_edid_duplicate(). Correct?
> 

Correct. The only other "good" place would be in the `if (new_edid)` conditional,
but that wouldn't be as readable as it is right now...

Cheers,
Angelo
  

On 12/04/2023 10:06, AngeloGioacchino Del Regno wrote:
> Il 12/04/23 09:08, Matthias Brugger ha scritto:
>>
>>
>> On 04/04/2023 12:47, AngeloGioacchino Del Regno wrote:
>>> Since eDP panels are not removable it is safe to cache the EDID:
>>> this will avoid a relatively long read transaction at every PM
>>> resume that is unnecessary only in the "special" case of eDP,
>>> hence speeding it up a little, as from now on, as resume operation,
>>> we will perform only link training.
>>>
>>> Signed-off-by: AngeloGioacchino Del Regno 
>>> <angelogioacchino.delregno@collabora.com>
>>> ---
>>>   drivers/gpu/drm/mediatek/mtk_dp.c | 11 ++++++++++-
>>>   1 file changed, 10 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/mediatek/mtk_dp.c 
>>> b/drivers/gpu/drm/mediatek/mtk_dp.c
>>> index 1f94fcc144d3..84f82cc68672 100644
>>> --- a/drivers/gpu/drm/mediatek/mtk_dp.c
>>> +++ b/drivers/gpu/drm/mediatek/mtk_dp.c
>>> @@ -118,6 +118,7 @@ struct mtk_dp {
>>>       const struct mtk_dp_data *data;
>>>       struct mtk_dp_info info;
>>>       struct mtk_dp_train_info train_info;
>>> +    struct edid *edid;
>>>       struct platform_device *phy_dev;
>>>       struct phy *phy;
>>> @@ -1993,7 +1994,11 @@ static struct edid *mtk_dp_get_edid(struct drm_bridge 
>>> *bridge,
>>>           usleep_range(2000, 5000);
>>>       }
>>> -    new_edid = drm_get_edid(connector, &mtk_dp->aux.ddc);
>>> +    /* eDP panels aren't removable, so we can return a cached EDID. */
>>> +    if (mtk_dp->edid && mtk_dp->bridge.type == DRM_MODE_CONNECTOR_eDP)

Maybe better like this:
if (mtk_dp->bridge.type == DRM_MODE_CONNECTOR_eDP && mtk_dp->edid)

To in sync with the if statement below. Anyway we are only concerned if it's an 
eDP so check that first (and hope the compiler will do so as well ;)

With that:
Reviewed-by: Matthias Brugger <matthias.bgg@gmail.com>

>>> +        new_edid = drm_edid_duplicate(mtk_dp->edid);
>>> +    else
>>> +        new_edid = drm_get_edid(connector, &mtk_dp->aux.ddc);
>>
>> Maybe it would make sense to add a macro for the check of mtk_dp->bridge.type 
>> == DRM_MODE_CONNECTOR_eDP
>> it would make the code more readable.
>>
> 
> I had the same idea... but then avoided that because in most (if not all?) of the
> DRM drivers (at least, the one I've read) this check is always open coded, so I
> wrote it like that for consistency and nothing else.
> 
> I have no strong opinions on that though!
> 

I think the only reasonable solution would be a macro like:
DRM_CONNECTOR_MODE_IS(mtk_dp->bridge.type, eDP) which in the end is longer then 
open-code it, so probably just leave it as it is.

>>>       /*
>>>        * Parse capability here to let atomic_get_input_bus_fmts and
>>> @@ -2022,6 +2027,10 @@ static struct edid *mtk_dp_get_edid(struct drm_bridge 
>>> *bridge,
>>>           drm_atomic_bridge_chain_post_disable(bridge, connector->state->state);
>>>       }
>>> +    /* If this is an eDP panel and the read EDID is good, cache it for later */
>>> +    if (mtk_dp->bridge.type == DRM_MODE_CONNECTOR_eDP && !mtk_dp->edid && 
>>> new_edid)
>>> +        mtk_dp->edid = drm_edid_duplicate(new_edid);
>>> +
>>
>> How about putting this in an else if branch of mtk_dp_parse_capabilities. At 
>> least we could get rid of the check regarding if new_edid != NULL.
>>
>> I was thinking on how to put both if statements in one block, but I think the 
>> problem is, that we would leak memory if the capability parsing failes due to 
>> the call to drm_edid_duplicate(). Correct?
>>
> 
> Correct. The only other "good" place would be in the `if (new_edid)` conditional,
> but that wouldn't be as readable as it is right now...
> 
> Cheers,
> Angelo
>