Re: [PATCH 08/11] drm/msm/dp: switch to struct drm_edid

[Jani Nikula <jani.nikula@intel.com>]

On Sun, 19 May 2024, Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote:
> On Tue, May 14, 2024 at 03:55:14PM +0300, Jani Nikula wrote:
>> Prefer the struct drm_edid based functions for reading the EDID and
>> updating the connector.
>> 
>> Simplify the flow by updating the EDID property when the EDID is read
>> instead of at .get_modes.
>> 
>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> 
>> ---
>
> The patch looks good to me, I'd like to hear an opinion from Doug (added
> to CC).
>
> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

Thanks!

> What is the merge strategy for the series? Do you plan to pick up all
> the patches to drm-misc or should we pick up individual patches into
> driver trees?

I think all of the patches here are connected in theme, but
independent. Either way is fine by me.

>
>
>> 
>> Cc: Rob Clark <robdclark@gmail.com>
>> Cc: Abhinav Kumar <quic_abhinavk@quicinc.com>
>> Cc: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>> Cc: Sean Paul <sean@poorly.run>
>> Cc: Marijn Suijten <marijn.suijten@somainline.org>
>> Cc: linux-arm-msm@vger.kernel.org
>> Cc: freedreno@lists.freedesktop.org
>> ---
>>  drivers/gpu/drm/msm/dp/dp_display.c | 11 +++----
>>  drivers/gpu/drm/msm/dp/dp_panel.c   | 47 +++++++++--------------------
>>  drivers/gpu/drm/msm/dp/dp_panel.h   |  2 +-
>>  3 files changed, 20 insertions(+), 40 deletions(-)
>
> [skipped]
>
>> @@ -249,10 +228,12 @@ void dp_panel_handle_sink_request(struct dp_panel *dp_panel)
>>  	panel = container_of(dp_panel, struct dp_panel_private, dp_panel);
>>  
>>  	if (panel->link->sink_request & DP_TEST_LINK_EDID_READ) {
>> +		/* FIXME: get rid of drm_edid_raw() */
>
> The code here can get use of something like drm_edid_smth_checksum().
> 'Something', because I could not come up with the word that would make
> it clear that it is the declared checksum instead of the actual /
> computed one.

This is an annoying one, to be honest, and linked to the historical fact
that we filter some EDID blocks that have an incorrect checksum.

(Some blocks, yes. We don't filter all blocks, because there are some
nasty docks out there that modify the CTA block but fail to update the
checksum, and filtering the CTA blocks would render the display
useless. So we accept CTA blocks with incorrect checksums. But reject
others. Yay.)

IMO the real fix would be to stop mucking with the EDID, and just expose
it to userspace, warts and all. We could still ignore the EDID blocks
with incorrect checksum while using it ourselves if we want to. And with
that, we could just have a function that checks the last EDID block's
checksum, and stop using this ->real_edid_checksum thing.

Anyway, yes, we could add the function already.

BR,
Jani.

>
>> +		const struct edid *edid = drm_edid_raw(dp_panel->drm_edid);
>>  		u8 checksum;
>>  
>> -		if (dp_panel->edid)
>> -			checksum = dp_panel_get_edid_checksum(dp_panel->edid);
>> +		if (edid)
>> +			checksum = dp_panel_get_edid_checksum(edid);
>>  		else
>>  			checksum = dp_panel->connector->real_edid_checksum;
>>  

-- 
Jani Nikula, Intel