The flux capacitor stopped fluxing...
On Fri, Nov 3, 2023, 5:06 AM Hans Verkuil <hverk...@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 <mrip...@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);
> > }
> >
>
>
