Re: [PATCH 04/45] drm/msm/dp: split msm_dp_panel_read_sink_caps() into two parts

[Yongxing Mou <quic_yongmou@quicinc.com>]

On 2024/12/6 16:51, Dmitry Baryshkov wrote:
> On Thu, Dec 05, 2024 at 08:31:35PM -0800, Abhinav Kumar wrote:
>> In preparation of DP MST where link caps are read for the
>> immediate downstream device and the edid is read through
>> sideband messaging, split the msm_dp_panel_read_sink_caps() into
>> two parts which read the link parameters and the edid parts
>> respectively.
> 
> As you are touching this part, could you please refactor the code
> instead by dropping the msm_dp_panel->drm_edid? There should be no need
> to store EDID in the panel structure.
> 
Hi, Dmitry, Abhinav will be leaving the company and will no longer be 
responsible for updating and address the comments. I will take over 
handling MST patch series. Regarding this comments, I don't got that 
where the drm_edid should be stored. In MST cases, where multiple panels 
exist, i think that there should be a separate drm_edid saved for each 
panel.
>>
>> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
>> ---
>>   drivers/gpu/drm/msm/dp/dp_display.c |  6 +++++-
>>   drivers/gpu/drm/msm/dp/dp_panel.c   | 25 +++++++++++++++++--------
>>   drivers/gpu/drm/msm/dp/dp_panel.h   |  5 ++++-
>>   3 files changed, 26 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
>> index be26064af9febf4f4761e21ea7db85ab1ac66081..052db80c6a365f53c2c0a37d3b69ea2b627aea1f 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
>> @@ -372,7 +372,11 @@ static int msm_dp_display_process_hpd_high(struct msm_dp_display_private *dp)
>>   	const struct drm_display_info *info = &connector->display_info;
>>   	int rc = 0;
>>   
>> -	rc = msm_dp_panel_read_sink_caps(dp->panel, connector);
>> +	rc = msm_dp_panel_read_link_caps(dp->panel, connector);
>> +	if (rc)
>> +		goto end;
>> +
>> +	rc = msm_dp_panel_read_edid(dp->panel, connector);
>>   	if (rc)
>>   		goto end;
>>   
>> diff --git a/drivers/gpu/drm/msm/dp/dp_panel.c b/drivers/gpu/drm/msm/dp/dp_panel.c
>> index 5d7eaa31bf3176566f40f01ff636bee64e81c64f..d277e9b2cbc03688976b6aa481ee724b186bab51 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_panel.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_panel.c
>> @@ -108,8 +108,8 @@ static u32 msm_dp_panel_get_supported_bpp(struct msm_dp_panel *msm_dp_panel,
>>   	return min_supported_bpp;
>>   }
>>   
>> -int msm_dp_panel_read_sink_caps(struct msm_dp_panel *msm_dp_panel,
>> -	struct drm_connector *connector)
>> +int msm_dp_panel_read_link_caps(struct msm_dp_panel *msm_dp_panel,
>> +				struct drm_connector *connector)
> 
> This function doesn't require connector anymore.
Right, will remove connector in function.
> 
>>   {
>>   	int rc, bw_code;
>>   	int count;
>> @@ -150,8 +150,19 @@ int msm_dp_panel_read_sink_caps(struct msm_dp_panel *msm_dp_panel,
>>   
>>   	rc = drm_dp_read_downstream_info(panel->aux, msm_dp_panel->dpcd,
>>   					 msm_dp_panel->downstream_ports);
>> -	if (rc)
>> -		return rc;
>> +	return rc;
>> +}
>> +
>> +int msm_dp_panel_read_edid(struct msm_dp_panel *msm_dp_panel, struct drm_connector *connector)
>> +{
>> +	struct msm_dp_panel_private *panel;
>> +
>> +	if (!msm_dp_panel || !connector) {
>> +		DRM_ERROR("invalid input\n");
>> +		return -EINVAL;
>> +	}
> 
> Neither panel nor connector can be NULL here, please drop.
> 
>> +
>> +	panel = container_of(msm_dp_panel, struct msm_dp_panel_private, msm_dp_panel);
>>   
>>   	drm_edid_free(msm_dp_panel->drm_edid);
>>   
>> @@ -163,13 +174,11 @@ int msm_dp_panel_read_sink_caps(struct msm_dp_panel *msm_dp_panel,
>>   		DRM_ERROR("panel edid read failed\n");
>>   		/* check edid read fail is due to unplug */
>>   		if (!msm_dp_catalog_link_is_connected(panel->catalog)) {
>> -			rc = -ETIMEDOUT;
>> -			goto end;
>> +			return -ETIMEDOUT;
>>   		}
>>   	}
>>   
>> -end:
>> -	return rc;
>> +	return 0;
>>   }
>>   
>>   u32 msm_dp_panel_get_mode_bpp(struct msm_dp_panel *msm_dp_panel,
>> diff --git a/drivers/gpu/drm/msm/dp/dp_panel.h b/drivers/gpu/drm/msm/dp/dp_panel.h
>> index 0e944db3adf2f187f313664fe80cf540ec7a19f2..7a38655c443af597c84fb78c6702b2a3ef9822ed 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_panel.h
>> +++ b/drivers/gpu/drm/msm/dp/dp_panel.h
>> @@ -59,7 +59,10 @@ void msm_dp_panel_dump_regs(struct msm_dp_panel *msm_dp_panel);
>>   int msm_dp_panel_read_sink_caps(struct msm_dp_panel *msm_dp_panel,
>>   		struct drm_connector *connector);
>>   u32 msm_dp_panel_get_mode_bpp(struct msm_dp_panel *msm_dp_panel, u32 mode_max_bpp,
>> -			u32 mode_pclk_khz);
>> +			      u32 mode_pclk_khz);
>> +int msm_dp_panel_read_link_caps(struct msm_dp_panel *dp_panel,
>> +				struct drm_connector *connector);
>> +int msm_dp_panel_read_edid(struct msm_dp_panel *dp_panel, struct drm_connector *connector);
>>   int msm_dp_panel_get_modes(struct msm_dp_panel *msm_dp_panel,
>>   		struct drm_connector *connector);
>>   void msm_dp_panel_handle_sink_request(struct msm_dp_panel *msm_dp_panel);
>>
>> -- 
>> 2.34.1
>>
>