Re: [PATCH 1/2] drm/bridge: add ->edid_read hook and drm_bridge_edid_read()

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

On Fri, 22 Dec 2023, Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote:
> On Thu, 26 Oct 2023 at 12:40, Jani Nikula <jani.nikula@intel.com> wrote:
>>
>> Add new struct drm_edid based ->edid_read hook and
>> drm_bridge_edid_read() function to call the hook.
>>
>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> ---
>>  drivers/gpu/drm/drm_bridge.c | 46 +++++++++++++++++++++++++++++++++++-
>>  include/drm/drm_bridge.h     | 33 ++++++++++++++++++++++++++
>>  2 files changed, 78 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
>> index 30d66bee0ec6..e1cfba2ff583 100644
>> --- a/drivers/gpu/drm/drm_bridge.c
>> +++ b/drivers/gpu/drm/drm_bridge.c
>> @@ -27,8 +27,9 @@
>>  #include <linux/mutex.h>
>>
>>  #include <drm/drm_atomic_state_helper.h>
>> -#include <drm/drm_debugfs.h>
>>  #include <drm/drm_bridge.h>
>> +#include <drm/drm_debugfs.h>
>> +#include <drm/drm_edid.h>
>>  #include <drm/drm_encoder.h>
>>  #include <drm/drm_file.h>
>>  #include <drm/drm_of.h>
>> @@ -1206,6 +1207,47 @@ int drm_bridge_get_modes(struct drm_bridge *bridge,
>>  }
>>  EXPORT_SYMBOL_GPL(drm_bridge_get_modes);
>>
>> +/**
>> + * drm_bridge_edid_read - read the EDID data of the connected display
>> + * @bridge: bridge control structure
>> + * @connector: the connector to read EDID for
>> + *
>> + * If the bridge supports output EDID retrieval, as reported by the
>> + * DRM_BRIDGE_OP_EDID bridge ops flag, call &drm_bridge_funcs.edid_read to get
>> + * the EDID and return it. Otherwise return NULL.
>> + *
>> + * If &drm_bridge_funcs.edid_read is not set, fall back to using
>> + * drm_bridge_get_edid() and wrapping it in struct drm_edid.
>> + *
>> + * RETURNS:
>> + * The retrieved EDID on success, or NULL otherwise.
>
> Wouldn't it be better to return an ERR_PTR instead of NULL?

I don't think so. The existing drm_bridge_get_edid() returns NULL on
errors. The ->get_edid hook returns NULL on errors. The new ->edid_read
returns NULL on errors. The drm_get_edid() and drm_edid_read() functions
return NULL on errors.

It would be surprising if one of the functions started returning error
pointers.

I don't see any added benefit with error pointers here. The ->edid_read
hook could be made to return error pointers too, but then there's quite
the burden in making all drivers return sensible values where the
difference in error code actually matters. And which error scenarios do
we want to differentiate here? How should we handle them differently?


BR,
Jani.


>
>> + */
>> +const struct drm_edid *drm_bridge_edid_read(struct drm_bridge *bridge,
>> +                                           struct drm_connector *connector)
>> +{
>> +       if (!(bridge->ops & DRM_BRIDGE_OP_EDID))
>> +               return NULL;
>> +
>> +       /* Transitional: Fall back to ->get_edid. */
>> +       if (!bridge->funcs->edid_read) {
>> +               const struct drm_edid *drm_edid;
>> +               struct edid *edid;
>> +
>> +               edid = drm_bridge_get_edid(bridge, connector);
>> +               if (!edid)
>> +                       return NULL;
>> +
>> +               drm_edid = drm_edid_alloc(edid, (edid->extensions + 1) * EDID_LENGTH);
>> +
>> +               kfree(edid);
>> +
>> +               return drm_edid;
>> +       }
>> +
>> +       return bridge->funcs->edid_read(bridge, connector);
>> +}
>> +EXPORT_SYMBOL_GPL(drm_bridge_edid_read);
>
> [skipped the rest]

-- 
Jani Nikula, Intel