[PATCH RFC v3 12/37] drm/connector: hdmi: Create Infoframe DebugFS entries

[Jeff Hennessey]

The flux capacitor stopped fluxing...

On Fri, Nov 3, 2023, 5:06 AM Hans Verkuil <hverkuil at xs4all.nl> wrote:

> Hi Maxime,
>
> Thank you for posting v3, this time it runs fine on my RPi 4, thank you for
> fixing that.
>
> I'll start working on a conformity checker for this.
>
> I have a few remarks:
>
> On 31/10/2023 17:48, Maxime Ripard wrote:
> > There has been some discussions recently about the infoframes sent by
> > drivers and if they were properly generated.
> >
> > In parallel, there's been some interest in creating an infoframe-decode
> > tool similar to edid-decode.
> >
> > Both would be much easier if we were to expose the infoframes programmed
> > in the hardware. It won't be perfect since we have no guarantee that
> > it's actually what goes through the wire, but it's the best we can do.
> >
> > Signed-off-by: Maxime Ripard <mripard at kernel.org>
> > ---
> >  drivers/gpu/drm/drm_debugfs.c | 110
> ++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 110 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_debugfs.c
> b/drivers/gpu/drm/drm_debugfs.c
> > index 2de43ff3ce0a..3c65b1d3f926 100644
> > --- a/drivers/gpu/drm/drm_debugfs.c
> > +++ b/drivers/gpu/drm/drm_debugfs.c
> > @@ -538,6 +538,114 @@ static const struct file_operations
> drm_connector_fops = {
> >       .write = connector_write
> >  };
> >
> > +struct debugfs_wrapper {
> > +     struct drm_connector *connector;
> > +     struct drm_connector_hdmi_infoframe *frame;
> > +};
> > +
> > +#define HDMI_MAX_INFOFRAME_SIZE              29
> > +
> > +static ssize_t
> > +infoframe_read(struct file *filp, char __user *ubuf, size_t count,
> loff_t *ppos)
> > +{
> > +     const struct debugfs_wrapper *wrapper = filp->private_data;
> > +     struct drm_connector *connector = wrapper->connector;
> > +     struct drm_connector_hdmi_infoframe *infoframe = wrapper->frame;
> > +     union hdmi_infoframe *frame = &infoframe->data;
> > +     u8 buf[HDMI_MAX_INFOFRAME_SIZE];
> > +     ssize_t len = 0;
> > +
> > +     mutex_lock(&connector->hdmi.infoframes.lock);
> > +
> > +     if (!infoframe->set)
> > +             goto out;
> > +
> > +     len = hdmi_infoframe_pack(frame, buf, sizeof(buf));
> > +     if (len < 0)
> > +             goto out;
> > +
> > +     len = simple_read_from_buffer(ubuf, count, ppos, buf, len);
> > +
> > +out:
> > +     mutex_unlock(&connector->hdmi.infoframes.lock);
> > +     return len;
> > +}
> > +
> > +static const struct file_operations infoframe_fops = {
> > +     .owner   = THIS_MODULE,
> > +     .open    = simple_open,
> > +     .read    = infoframe_read,
> > +};
> > +
> > +static int create_hdmi_infoframe_file(struct drm_connector *connector,
> > +                                   struct dentry *parent,
> > +                                   const char *filename,
> > +                                   struct drm_connector_hdmi_infoframe
> *frame)
> > +{
> > +     struct drm_device *dev = connector->dev;
> > +     struct debugfs_wrapper *wrapper;
> > +     struct dentry *file;
> > +
> > +     wrapper = drmm_kzalloc(dev, sizeof(*wrapper), GFP_KERNEL);
> > +     if (!wrapper)
> > +             return -ENOMEM;
> > +
> > +     wrapper->connector = connector;
> > +     wrapper->frame = frame;
> > +
> > +     file = debugfs_create_file(filename, 0400, parent, wrapper,
> &infoframe_fops);
> > +     if (IS_ERR(file))
> > +             return PTR_ERR(file);
> > +
> > +     return 0;
> > +}
> > +
> > +#define CREATE_HDMI_INFOFRAME_FILE(c, p, i)          \
> > +     create_hdmi_infoframe_file(c, p, #i, &(c)->hdmi.infoframes.i)
> > +
> > +static int create_hdmi_infoframe_files(struct drm_connector *connector,
> > +                                    struct dentry *parent)
> > +{
> > +     int ret;
> > +
> > +     ret = CREATE_HDMI_INFOFRAME_FILE(connector, parent, audio);
> > +     if (ret)
> > +             return ret;
> > +
> > +     ret = CREATE_HDMI_INFOFRAME_FILE(connector, parent, avi);
> > +     if (ret)
> > +             return ret;
> > +
> > +     ret = CREATE_HDMI_INFOFRAME_FILE(connector, parent, drm);
>
> Hmm, I had to look into the code to figure out that 'drm' stands for
> Dynamic Range and Mastering InfoFrame. While entirely correct, it is
> also very confusing in the context of the 'drm' subsystem.
>
> I am not quite certain what the best approach is here.
>
> Internally in the drm code it is talking about 'hdr' or 'hdr metadata',
> but that's a bit confusing as well since there is also an HDR Dynamic
> Metadata Extended InfoFrame defined in CTA-861, even though support for
> that is not (yet) implemented in drm.
>
> At minimum there should be a comment in the code explaining what drm
> stands for in this context.
>
> One option to consider is renaming this file to hdr_drm, thus indicating
> that this is HDR related.
>
> > +     if (ret)
> > +             return ret;
> > +
> > +     ret = CREATE_HDMI_INFOFRAME_FILE(connector, parent, spd);
> > +     if (ret)
> > +             return ret;
> > +
> > +     ret = CREATE_HDMI_INFOFRAME_FILE(connector, parent, vendor);
>
> There may be multiple vendor specific InfoFrames in the future, so how
> would that be handled? Perhaps add a comment here that currently only one
> vendor specific InfoFrame is supported, but suggest how to handle multiple
> VSIFs in the future.
>
> What would actually be nice (although probably not that easy to fix) is if
> the name of the file would be "vendor-XXXXXX' where 'XXXXXX' is the IEEE
> OUI
> number.
>
> Regards,
>
>         Hans
>
> > +     if (ret)
> > +             return ret;
> > +
> > +     return 0;
> > +}
> > +
> > +static void hdmi_debugfs_add(struct drm_connector *connector)
> > +{
> > +     struct dentry *dir;
> > +
> > +     if (!(connector->connector_type == DRM_MODE_CONNECTOR_HDMIA ||
> > +           connector->connector_type == DRM_MODE_CONNECTOR_HDMIB))
> > +             return;
> > +
> > +     dir = debugfs_create_dir("infoframes", connector->debugfs_entry);
> > +     if (IS_ERR(dir))
> > +             return;
> > +
> > +     create_hdmi_infoframe_files(connector, dir);
> > +}
> > +
> >  void drm_debugfs_connector_add(struct drm_connector *connector)
> >  {
> >       struct drm_minor *minor = connector->dev->primary;
> > @@ -565,6 +673,8 @@ void drm_debugfs_connector_add(struct drm_connector
> *connector)
> >       debugfs_create_file("output_bpc", 0444, root, connector,
> >                           &output_bpc_fops);
> >
> > +     hdmi_debugfs_add(connector);
> > +
> >       if (connector->funcs->debugfs_init)
> >               connector->funcs->debugfs_init(connector, root);
> >  }
> >
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20231107/de639ca9/attachment.htm>