Re: [PATCH 02/11] drm/sti/sti_hdmi: convert to struct drm_edid
On Thu, Aug 15, 2024 at 03:55:30PM +0300, Jani Nikula wrote:
> On Mon, 24 Jun 2024, Daniel Vetter wrote:
> > On Tue, May 14, 2024 at 03:55:08PM +0300, Jani Nikula wrote:
> >> Prefer the struct drm_edid based functions for reading the EDID and
> >> updating the connector.
> >>
> >> The functional change is that the CEC physical address gets invalidated
> >> when the EDID could not be read.
> >>
> >> Signed-off-by: Jani Nikula
> >>
> >> ---
> >>
> >> Cc: Alain Volmat
> >> Cc: Maarten Lankhorst
> >> Cc: Maxime Ripard
> >> Cc: Thomas Zimmermann
> >> ---
> >> drivers/gpu/drm/sti/sti_hdmi.c | 24 ++--
> >> 1 file changed, 14 insertions(+), 10 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/sti/sti_hdmi.c
> >> b/drivers/gpu/drm/sti/sti_hdmi.c
> >> index 500936d5743c..3b62ec2d742f 100644
> >> --- a/drivers/gpu/drm/sti/sti_hdmi.c
> >> +++ b/drivers/gpu/drm/sti/sti_hdmi.c
> >> @@ -974,28 +974,32 @@ static const struct drm_bridge_funcs
> >> sti_hdmi_bridge_funcs = {
> >>
> >> static int sti_hdmi_connector_get_modes(struct drm_connector *connector)
> >> {
> >> + const struct drm_display_info *info = &connector->display_info;
> >>struct sti_hdmi_connector *hdmi_connector
> >>= to_sti_hdmi_connector(connector);
> >>struct sti_hdmi *hdmi = hdmi_connector->hdmi;
> >> - struct edid *edid;
> >> + const struct drm_edid *drm_edid;
> >>int count;
> >>
> >>DRM_DEBUG_DRIVER("\n");
> >>
> >> - edid = drm_get_edid(connector, hdmi->ddc_adapt);
> >> - if (!edid)
> >> - goto fail;
> >> + drm_edid = drm_edid_read_ddc(connector, hdmi->ddc_adapt);
> >
> > I think you can use drm_edid_read here since the ddc is correctly set up
> > with drm_connector_init_with_ddc()
> >
> >> +
> >> + drm_edid_connector_update(connector, drm_edid);
> >>
> >> - cec_notifier_set_phys_addr_from_edid(hdmi->notifier, edid);
> >> + cec_notifier_set_phys_addr(hdmi->notifier,
> >> +
> >> connector->display_info.source_physical_address);
> >> +
> >> + if (!drm_edid)
> >> + goto fail;
> >
> > Unless I missed something, all the functions can cope with a NULL edid,
> > but this jump means in the failure case you'll return stack garbage in
> > count. Just drop this check?
>
> Reviving an old thread... the goto fail path returns 0, not count.
Oops, I stand corrected. With just drm_edid_read used:
Reviewed-by: Daniel Vetter
>
> BR,
> Jani.
>
>
> >
> >>
> >> - count = drm_add_edid_modes(connector, edid);
> >> - drm_connector_update_edid_property(connector, edid);
> >> + count = drm_edid_connector_add_modes(connector);
> >>
> >>DRM_DEBUG_KMS("%s : %dx%d cm\n",
> >> -(connector->display_info.is_hdmi ? "hdmi monitor" : "dvi
> >> monitor"),
> >> -edid->width_cm, edid->height_cm);
> >> +info->is_hdmi ? "hdmi monitor" : "dvi monitor",
> >> +info->width_mm / 10, info->height_mm / 10);
> >>
> >> - kfree(edid);
> >> + drm_edid_free(drm_edid);
> >>return count;
> >
> > With the two items addressed:
> >
> > Reviewed-by: Daniel Vetter
> >
> >>
> >> fail:
> >> --
> >> 2.39.2
> >>
>
> --
> Jani Nikula, Intel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
Re: [PATCH 02/11] drm/sti/sti_hdmi: convert to struct drm_edid
On Mon, 24 Jun 2024, Daniel Vetter wrote:
> On Tue, May 14, 2024 at 03:55:08PM +0300, Jani Nikula wrote:
>> Prefer the struct drm_edid based functions for reading the EDID and
>> updating the connector.
>>
>> The functional change is that the CEC physical address gets invalidated
>> when the EDID could not be read.
>>
>> Signed-off-by: Jani Nikula
>>
>> ---
>>
>> Cc: Alain Volmat
>> Cc: Maarten Lankhorst
>> Cc: Maxime Ripard
>> Cc: Thomas Zimmermann
>> ---
>> drivers/gpu/drm/sti/sti_hdmi.c | 24 ++--
>> 1 file changed, 14 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/sti/sti_hdmi.c b/drivers/gpu/drm/sti/sti_hdmi.c
>> index 500936d5743c..3b62ec2d742f 100644
>> --- a/drivers/gpu/drm/sti/sti_hdmi.c
>> +++ b/drivers/gpu/drm/sti/sti_hdmi.c
>> @@ -974,28 +974,32 @@ static const struct drm_bridge_funcs
>> sti_hdmi_bridge_funcs = {
>>
>> static int sti_hdmi_connector_get_modes(struct drm_connector *connector)
>> {
>> +const struct drm_display_info *info = &connector->display_info;
>> struct sti_hdmi_connector *hdmi_connector
>> = to_sti_hdmi_connector(connector);
>> struct sti_hdmi *hdmi = hdmi_connector->hdmi;
>> -struct edid *edid;
>> +const struct drm_edid *drm_edid;
>> int count;
>>
>> DRM_DEBUG_DRIVER("\n");
>>
>> -edid = drm_get_edid(connector, hdmi->ddc_adapt);
>> -if (!edid)
>> -goto fail;
>> +drm_edid = drm_edid_read_ddc(connector, hdmi->ddc_adapt);
>
> I think you can use drm_edid_read here since the ddc is correctly set up
> with drm_connector_init_with_ddc()
>
>> +
>> +drm_edid_connector_update(connector, drm_edid);
>>
>> -cec_notifier_set_phys_addr_from_edid(hdmi->notifier, edid);
>> +cec_notifier_set_phys_addr(hdmi->notifier,
>> +
>> connector->display_info.source_physical_address);
>> +
>> +if (!drm_edid)
>> +goto fail;
>
> Unless I missed something, all the functions can cope with a NULL edid,
> but this jump means in the failure case you'll return stack garbage in
> count. Just drop this check?
Reviving an old thread... the goto fail path returns 0, not count.
BR,
Jani.
>
>>
>> -count = drm_add_edid_modes(connector, edid);
>> -drm_connector_update_edid_property(connector, edid);
>> +count = drm_edid_connector_add_modes(connector);
>>
>> DRM_DEBUG_KMS("%s : %dx%d cm\n",
>> - (connector->display_info.is_hdmi ? "hdmi monitor" : "dvi
>> monitor"),
>> - edid->width_cm, edid->height_cm);
>> + info->is_hdmi ? "hdmi monitor" : "dvi monitor",
>> + info->width_mm / 10, info->height_mm / 10);
>>
>> -kfree(edid);
>> +drm_edid_free(drm_edid);
>> return count;
>
> With the two items addressed:
>
> Reviewed-by: Daniel Vetter
>
>>
>> fail:
>> --
>> 2.39.2
>>
--
Jani Nikula, Intel
Re: [PATCH 02/11] drm/sti/sti_hdmi: convert to struct drm_edid
On Tue, May 14, 2024 at 03:55:08PM +0300, Jani Nikula wrote:
> Prefer the struct drm_edid based functions for reading the EDID and
> updating the connector.
>
> The functional change is that the CEC physical address gets invalidated
> when the EDID could not be read.
>
> Signed-off-by: Jani Nikula
>
> ---
>
> Cc: Alain Volmat
> Cc: Maarten Lankhorst
> Cc: Maxime Ripard
> Cc: Thomas Zimmermann
> ---
> drivers/gpu/drm/sti/sti_hdmi.c | 24 ++--
> 1 file changed, 14 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/sti/sti_hdmi.c b/drivers/gpu/drm/sti/sti_hdmi.c
> index 500936d5743c..3b62ec2d742f 100644
> --- a/drivers/gpu/drm/sti/sti_hdmi.c
> +++ b/drivers/gpu/drm/sti/sti_hdmi.c
> @@ -974,28 +974,32 @@ static const struct drm_bridge_funcs
> sti_hdmi_bridge_funcs = {
>
> static int sti_hdmi_connector_get_modes(struct drm_connector *connector)
> {
> + const struct drm_display_info *info = &connector->display_info;
> struct sti_hdmi_connector *hdmi_connector
> = to_sti_hdmi_connector(connector);
> struct sti_hdmi *hdmi = hdmi_connector->hdmi;
> - struct edid *edid;
> + const struct drm_edid *drm_edid;
> int count;
>
> DRM_DEBUG_DRIVER("\n");
>
> - edid = drm_get_edid(connector, hdmi->ddc_adapt);
> - if (!edid)
> - goto fail;
> + drm_edid = drm_edid_read_ddc(connector, hdmi->ddc_adapt);
I think you can use drm_edid_read here since the ddc is correctly set up
with drm_connector_init_with_ddc()
> +
> + drm_edid_connector_update(connector, drm_edid);
>
> - cec_notifier_set_phys_addr_from_edid(hdmi->notifier, edid);
> + cec_notifier_set_phys_addr(hdmi->notifier,
> +
> connector->display_info.source_physical_address);
> +
> + if (!drm_edid)
> + goto fail;
Unless I missed something, all the functions can cope with a NULL edid,
but this jump means in the failure case you'll return stack garbage in
count. Just drop this check?
>
> - count = drm_add_edid_modes(connector, edid);
> - drm_connector_update_edid_property(connector, edid);
> + count = drm_edid_connector_add_modes(connector);
>
> DRM_DEBUG_KMS("%s : %dx%d cm\n",
> - (connector->display_info.is_hdmi ? "hdmi monitor" : "dvi
> monitor"),
> - edid->width_cm, edid->height_cm);
> + info->is_hdmi ? "hdmi monitor" : "dvi monitor",
> + info->width_mm / 10, info->height_mm / 10);
>
> - kfree(edid);
> + drm_edid_free(drm_edid);
> return count;
With the two items addressed:
Reviewed-by: Daniel Vetter
>
> fail:
> --
> 2.39.2
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
