Re: [PATCH 08/11] drm/msm/dp: switch to struct drm_edid
On Thu, 30 May 2024 at 15:45, Jani Nikula wrote: > > On Mon, 20 May 2024, Doug Anderson wrote: > > Hi, > > > > On Sun, May 19, 2024 at 2:01 AM Dmitry Baryshkov > > 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 > >> > > >> > --- > >> > >> The patch looks good to me, I'd like to hear an opinion from Doug (added > >> to CC). > >> > >> Reviewed-by: Dmitry Baryshkov > >> > >> 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'm not sure I have too much to add here aside from what you guys have > > already talked about. I'm OK with: > > > > Reviewed-by: Douglas Anderson > > I assume you'll want to pick this up for msm instead of me merging to > drm-misc. Yes, it's on my todo list. -- With best wishes Dmitry
Re: [PATCH 08/11] drm/msm/dp: switch to struct drm_edid
On Mon, 20 May 2024, Doug Anderson wrote: > Hi, > > On Sun, May 19, 2024 at 2:01 AM Dmitry Baryshkov > 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 >> > >> > --- >> >> The patch looks good to me, I'd like to hear an opinion from Doug (added >> to CC). >> >> Reviewed-by: Dmitry Baryshkov >> >> 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'm not sure I have too much to add here aside from what you guys have > already talked about. I'm OK with: > > Reviewed-by: Douglas Anderson I assume you'll want to pick this up for msm instead of me merging to drm-misc. BR, Jani. -- Jani Nikula, Intel
Re: [PATCH 08/11] drm/msm/dp: switch to struct drm_edid
Hi, On Sun, May 19, 2024 at 2:01 AM Dmitry Baryshkov 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 > > > > --- > > The patch looks good to me, I'd like to hear an opinion from Doug (added > to CC). > > Reviewed-by: Dmitry Baryshkov > > 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'm not sure I have too much to add here aside from what you guys have already talked about. I'm OK with: Reviewed-by: Douglas Anderson
Re: [PATCH 08/11] drm/msm/dp: switch to struct drm_edid
On Mon, 20 May 2024 at 15:25, Jani Nikula wrote:
>
> On Sun, 19 May 2024, Dmitry Baryshkov 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
> >>
> >> ---
> >
> > The patch looks good to me, I'd like to hear an opinion from Doug (added
> > to CC).
> >
> > Reviewed-by: Dmitry Baryshkov
>
> 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
> >> Cc: Abhinav Kumar
> >> Cc: Dmitry Baryshkov
> >> Cc: Sean Paul
> >> Cc: Marijn Suijten
> >> Cc: linux-arm-...@vger.kernel.org
> >> Cc: freedr...@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.
It is a part of the DP test suite if I remember correctly.
>
> (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
--
With best wishes
Dmitry
Re: [PATCH 08/11] drm/msm/dp: switch to struct drm_edid
On Sun, 19 May 2024, Dmitry Baryshkov 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
>>
>> ---
>
> The patch looks good to me, I'd like to hear an opinion from Doug (added
> to CC).
>
> Reviewed-by: Dmitry Baryshkov
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
>> Cc: Abhinav Kumar
>> Cc: Dmitry Baryshkov
>> Cc: Sean Paul
>> Cc: Marijn Suijten
>> Cc: linux-arm-...@vger.kernel.org
>> Cc: freedr...@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
Re: [PATCH 08/11] drm/msm/dp: switch to struct drm_edid
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
>
> ---
The patch looks good to me, I'd like to hear an opinion from Doug (added
to CC).
Reviewed-by: Dmitry Baryshkov
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?
>
> Cc: Rob Clark
> Cc: Abhinav Kumar
> Cc: Dmitry Baryshkov
> Cc: Sean Paul
> Cc: Marijn Suijten
> Cc: linux-arm-...@vger.kernel.org
> Cc: freedr...@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.
> + 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;
>
--
With best wishes
Dmitry
[PATCH 08/11] drm/msm/dp: switch to struct drm_edid
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
---
Cc: Rob Clark
Cc: Abhinav Kumar
Cc: Dmitry Baryshkov
Cc: Sean Paul
Cc: Marijn Suijten
Cc: linux-arm-...@vger.kernel.org
Cc: freedr...@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(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
b/drivers/gpu/drm/msm/dp/dp_display.c
index 672a7ba52eda..9622e58dce3e 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -360,26 +360,25 @@ static int dp_display_send_hpd_notification(struct
dp_display_private *dp,
static int dp_display_process_hpd_high(struct dp_display_private *dp)
{
+ struct drm_connector *connector = dp->dp_display.connector;
+ const struct drm_display_info *info = &connector->display_info;
int rc = 0;
- struct edid *edid;
- rc = dp_panel_read_sink_caps(dp->panel, dp->dp_display.connector);
+ rc = dp_panel_read_sink_caps(dp->panel, connector);
if (rc)
goto end;
dp_link_process_request(dp->link);
if (!dp->dp_display.is_edp)
- drm_dp_set_subconnector_property(dp->dp_display.connector,
+ drm_dp_set_subconnector_property(connector,
connector_status_connected,
dp->panel->dpcd,
dp->panel->downstream_ports);
- edid = dp->panel->edid;
-
dp->dp_display.psr_supported = dp->panel->psr_cap.version &&
psr_enabled;
- dp->audio_supported = drm_detect_monitor_audio(edid);
+ dp->audio_supported = info->has_audio;
dp_panel_handle_sink_request(dp->panel);
/*
diff --git a/drivers/gpu/drm/msm/dp/dp_panel.c
b/drivers/gpu/drm/msm/dp/dp_panel.c
index 07db8f37cd06..a916b5f3b317 100644
--- a/drivers/gpu/drm/msm/dp/dp_panel.c
+++ b/drivers/gpu/drm/msm/dp/dp_panel.c
@@ -108,28 +108,6 @@ static u32 dp_panel_get_supported_bpp(struct dp_panel
*dp_panel,
return bpp;
}
-static int dp_panel_update_modes(struct drm_connector *connector,
- struct edid *edid)
-{
- int rc = 0;
-
- if (edid) {
- rc = drm_connector_update_edid_property(connector, edid);
- if (rc) {
- DRM_ERROR("failed to update edid property %d\n", rc);
- return rc;
- }
- rc = drm_add_edid_modes(connector, edid);
- return rc;
- }
-
- rc = drm_connector_update_edid_property(connector, NULL);
- if (rc)
- DRM_ERROR("failed to update edid property %d\n", rc);
-
- return rc;
-}
-
int dp_panel_read_sink_caps(struct dp_panel *dp_panel,
struct drm_connector *connector)
{
@@ -175,12 +153,13 @@ int dp_panel_read_sink_caps(struct dp_panel *dp_panel,
if (rc)
return rc;
- kfree(dp_panel->edid);
- dp_panel->edid = NULL;
+ drm_edid_free(dp_panel->drm_edid);
+
+ dp_panel->drm_edid = drm_edid_read_ddc(connector, &panel->aux->ddc);
+
+ drm_edid_connector_update(connector, dp_panel->drm_edid);
- dp_panel->edid = drm_get_edid(connector,
- &panel->aux->ddc);
- if (!dp_panel->edid) {
+ if (!dp_panel->drm_edid) {
DRM_ERROR("panel edid read failed\n");
/* check edid read fail is due to unplug */
if (!dp_catalog_link_is_connected(panel->catalog)) {
@@ -224,13 +203,13 @@ int dp_panel_get_modes(struct dp_panel *dp_panel,
return -EINVAL;
}
- if (dp_panel->edid)
- return dp_panel_update_modes(connector, dp_panel->edid);
+ if (dp_panel->drm_edid)
+ return drm_edid_connector_add_modes(connector);
return 0;
}
-static u8 dp_panel_get_edid_checksum(struct edid *edid)
+static u8 dp_panel_get_edid_checksum(const struct edid *edid)
{
edid += edid->extensions;
@@ -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() */
+ 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);
