Re: [RFC 01/10] drm: ADV7511 i2c HDMI encoder driver
Hi Lars-Peter,
On Wednesday 04 September 2013 13:34:30 Lars-Peter Clausen wrote:
> [...]
>
> >> +
> >> +/**
> >> + * enum adv7511_input_color_depth - Selects the input format color depth
> >> + * @ADV7511_INPUT_COLOR_DEPTH_8BIT: Input format color depth is 8 bits
> >> per channel
> >> + * @ADV7511_INPUT_COLOR_DEPTH_10BIT: Input format color dpeth is 10 bits
> >> per channel
> >> + * @ADV7511_INPUT_COLOR_DEPTH_12BIT: Input format color depth is 12 bits
> >> per channel
> >> + **/
> >> +enum adv7511_input_color_depth {
> >> + ADV7511_INPUT_COLOR_DEPTH_8BIT = 3,
> >> + ADV7511_INPUT_COLOR_DEPTH_10BIT = 1,
> >> + ADV7511_INPUT_COLOR_DEPTH_12BIT = 2,
> >> +};
> >
> > Those enums describe the video format at the chip input. This can be
> > configured statically from platform data or DT, but some platforms might
> > need to setup formats dynamically at runtime. For instance the ADV7511
> > could be driven by a mux with two inputs using different formats.
> >
> > For these reasons I would combine all those enums in a single one that
> > lists all possible input formats. The formats should be standardized and
> > moved to a separate header file. Get and set format operations will be
> > needed (this is something CDF will address :-)).
>
> Since most these settings are orthogonal to each other putting them in one
> enum gives you 3 * 3 * 6 * 3 = 162 entries. These enums configure to which
> pins of the ADV7511 what signal is connected. Standardizing this would
> require that other chips use the same layouts for connecting the pins.
The problem is, this information needs to be shared between devices. Formats
on the bus could very well be dynamic, the ADV7511 video source could be able
to generate multiple formats that would be runtime configurable. To support
this, platform data and DT bindings should describe how signals are connected
(which would thus filter the list of supported formats), and a runtime API
will be needed to get and set formats. The formats thus need to be
standardized
We had a similar issue in V4L2 and have introduced a media bus format
enumeration to solve it (see include/uapi/linux/v4l2-mediabus.h). I believe
something similar is needed here.
Of course this won't solve the issue of how to select a format on the bus when
both endpoints support multiple formats. This should probably be selected by
userspace somehow, but we're missing an API to do so at the moment. A default
value would thus need to be computed somehow, and possibly given by platform
data and DT bindings (although it doesn't describe the hardware as such, and
should thus not be part of the DT bindings).
> >> +/**
> >> + * enum adv7511_up_conversion - Selects the upscaling conversion method
> >> + * @ADV7511_UP_CONVERSION_ZERO_ORDER: Use zero order up conversion
> >> + * @ADV7511_UP_CONVERSION_FIRST_ORDER: Use first order up conversion
> >> + *
> >> + * This used when converting from a 4:2:2 format to a 4:4:4 format.
> >> + **/
> >> +enum adv7511_up_conversion {
> >> +ADV7511_UP_CONVERSION_ZERO_ORDER = 0,
> >> +ADV7511_UP_CONVERSION_FIRST_ORDER = 1,
> >> +};
> >
> > How is the upscaling conversion method supposed to be selected ? What does
> > it depend on ?
>
> It's probably up to the system designer to say which method yields better
> results for their system.
And what would a system designer base his/her decision on ? :-)
> >> +/**
> >> + * struct adv7511_video_config - Describes adv7511 hardware
> >> configuration
> >> + * @csc_enable: Whether to enable color space conversion
> >> + * @csc_scaling_factor: Color space conversion scaling factor
> >> + * @csc_coefficents: Color space conversion coefficents
> >> + * @hdmi_mode:Whether to use HDMI or DVI output mode
> >> + * @avi_infoframe:HDMI infoframe
> >> + **/
> >> +struct adv7511_video_config {
> >> + bool csc_enable;
> >
> > Shouldn't this be automatically computed based on the input and output
> > formats ?
>
> If the driver knew the input and output colorspaces and had coefficient
> tables for all possible combinations built in that could be done. This is
> maybe something that could be done in the framework.
>
> >> + enum adv7511_csc_scaling csc_scaling_factor;
> >> + const uint16_t *csc_coefficents;
> >
> > Do the coefficients need to be configured freely, or could presets do ?
> >
> >> + bool hdmi_mode;
> >> + struct hdmi_avi_infoframe avi_infoframe;
> >> +};
>
> [...]
>
> >> +static void adv7511_set_config(struct drm_encoder *encoder, void *c)
> >
> > Now we're getting to the controversial point.
> >
> > What bothers me about the DRM encoder slave API is that the display
> > controller driver needs to be aware of the details of the slave encoder,
> > as it needs to pass an encoder-specific configuration structure to the
> > .set_config() operation. The question would thus be, what about using the
> > Common Display Framework ?
>
> Well, as I s
Re: [RFC 01/10] drm: ADV7511 i2c HDMI encoder driver
[...]
>> +
>> +/**
>> + * enum adv7511_input_color_depth - Selects the input format color depth
>> + * @ADV7511_INPUT_COLOR_DEPTH_8BIT: Input format color depth is 8 bits per
> channel
>> + * @ADV7511_INPUT_COLOR_DEPTH_10BIT: Input format color dpeth is 10 bits
> per channel
>> + * @ADV7511_INPUT_COLOR_DEPTH_12BIT: Input format color depth is 12 bits
> per channel
>> + **/
>> +enum adv7511_input_color_depth {
>> +ADV7511_INPUT_COLOR_DEPTH_8BIT = 3,
>> +ADV7511_INPUT_COLOR_DEPTH_10BIT = 1,
>> +ADV7511_INPUT_COLOR_DEPTH_12BIT = 2,
>> +};
>
> Those enums describe the video format at the chip input. This can be
> configured statically from platform data or DT, but some platforms might need
> to setup formats dynamically at runtime. For instance the ADV7511 could be
> driven by a mux with two inputs using different formats.
>
> For these reasons I would combine all those enums in a single one that lists
> all possible input formats. The formats should be standardized and moved to a
> separate header file. Get and set format operations will be needed (this is
> something CDF will address :-)).
Since most these settings are orthogonal to each other putting them in one
enum gives you 3 * 3 * 6 * 3 = 162 entries. These enums configure to which
pins of the ADV7511 what signal is connected. Standardizing this would
require that other chips use the same layouts for connecting the pins.
[...]
>> +
>> +/**
>> + * enum adv7511_up_conversion - Selects the upscaling conversion method
>> + * @ADV7511_UP_CONVERSION_ZERO_ORDER: Use zero order up conversion
>> + * @ADV7511_UP_CONVERSION_FIRST_ORDER: Use first order up conversion
>> + *
>> + * This used when converting from a 4:2:2 format to a 4:4:4 format.
>> + **/
>> +enum adv7511_up_conversion {
>> +ADV7511_UP_CONVERSION_ZERO_ORDER = 0,
>> +ADV7511_UP_CONVERSION_FIRST_ORDER = 1,
>> +};
>
> How is the upscaling conversion method supposed to be selected ? What does it
> depend on ?
>
It's probably up to the system designer to say which method yields better
results for their system.
[...]
>> +/**
>> + * struct adv7511_video_config - Describes adv7511 hardware configuration
>> + * @csc_enable: Whether to enable color space conversion
>> + * @csc_scaling_factor: Color space conversion scaling factor
>> + * @csc_coefficents:Color space conversion coefficents
>> + * @hdmi_mode: Whether to use HDMI or DVI output mode
>> + * @avi_infoframe: HDMI infoframe
>> + **/
>> +struct adv7511_video_config {
>> +bool csc_enable;
>
> Shouldn't this be automatically computed based on the input and output
> formats
> ?
If the driver knew the input and output colorspaces and had coefficient
tables for all possible combinations built in that could be done. This is
maybe something that could be done in the framework.
>
>> +enum adv7511_csc_scaling csc_scaling_factor;
>> +const uint16_t *csc_coefficents;
>
> Do the coefficients need to be configured freely, or could presets do ?
>
>> +bool hdmi_mode;
>> +struct hdmi_avi_infoframe avi_infoframe;
>> +};
[...]
>> +static void adv7511_set_config(struct drm_encoder *encoder, void *c)
>
> Now we're getting to the controversial point.
>
> What bothers me about the DRM encoder slave API is that the display
> controller
> driver needs to be aware of the details of the slave encoder, as it needs to
> pass an encoder-specific configuration structure to the .set_config()
> operation. The question would thus be, what about using the Common Display
> Framework ?
Well, as I said I'd prefer using the CDF for this driver. But even then the
display controller driver might need to know about the details of the
encoder. I think we talked about this during the FOSDEM meeting. The
graphics fabric on the board can easily get complex enough to require a
board custom driver, similar to what we have in ASoC. I think this
drm_bridge patchset[1] tries to address a similar issue.
[1] http://lists.freedesktop.org/archives/dri-devel/2013-August/043237.html
[...]
>> +
>> +for (i = 0; i < 4; ++i) {
>> +ret = i2c_transfer(adv7511->i2c_edid->adapter, xfer,
> ARRAY_SIZE(xfer));
>> +if (ret < 0)
>> +return ret;
>> +else if (ret != 2)
>> +return -EIO;
>> +
>> +xfer[1].buf += 64;
>> +offset += 64;
>> +}
>
> Shouldn't you read two times 64 bytes per block, not four times ?
The controller on the ADV7511 always reads two blocks at once, so it is 256
bytes.
>
>> +
>> +adv7511->current_edid_segment = block / 2;
>> +}
>> +
>> +if (block % 2 == 0)
>> +memcpy(buf, adv7511->edid_buf, len);
>> +else
>> +memcpy(buf, adv7511->edid_buf + 128, len);
>> +
>> +return 0;
>> +}
>> +
[...]
>> +
>> +struct edid *adv7511_get_ed
Re: [RFC 01/10] drm: ADV7511 i2c HDMI encoder driver
Hi Lars-Peter, (CC'ing Hans Verkuil and the dri-devel and linux-media mailing lists) On Monday 02 September 2013 17:09:11 Lars-Peter Clausen wrote: > On 09/02/2013 05:01 PM, Laurent Pinchart wrote: > > On Monday 02 September 2013 16:43:25 Lars-Peter Clausen wrote: > >> On 09/02/2013 04:15 PM, Laurent Pinchart wrote: > >>> On Monday 02 September 2013 15:40:22 Lars-Peter Clausen wrote: > On 09/02/2013 03:18 PM, Laurent Pinchart wrote: > > On Friday 30 August 2013 14:37:35 Ulrich Hecht wrote: > >> ADV7511 driver snapshot taken from commit f416e32 of xcomm_zynq_3_10 > >> branch at https://github.com/analogdevicesinc/linux.git > > > > I believe Lars-Peter (CC'ed) was planning to upstream the driver. > > Lars-Peter, could you please share your plans ? > > My plan was to have all this upstream long time ago ;) > > As I said in that other thread, I don't think it is a good idea to have > two drivers for the same device. But if nobody else sees a problem with > this and as long as the v4l driver doesn't have devicetree support I > guess we could get away with it for now. Even if it will probably haunt > us later on. > > There are still a few minor bits and pieces to iron out, but lets try > to aim for 2.6.13. > >>> > >>> If you can make it for 2.6.13 I will be extremely impressed :-) > >> > >> Yea, I know DRM always takes a bit longer... > > > > I think you meant 3.13 ;-) > > > >>> Do you plan to push the driver yourself ? If so, I would appreciate if > >>> you could CC me. If there's just a few minor bits to iron out I can > >>> already review your latest version if that can help. > >> > >> There are a couple of style issues, so if you review the driver ignore > >> these for now, but otherwise feedback is welcome, thanks. And I'm not > >> also quite happy with the ASoC integration yet. I'll thus concentrate on the video part in the review. Any chance to get the video portion to mainline first and then add audio support ? > > Sure. Is the version available from the above branch the latest version ? > > Yes, you can find it here: > https://github.com/analogdevicesinc/linux/tree/xcomm_zynq/drivers/gpu/drm/ > i2c Thank you. > Oh and the DT bindings also still need proper documentation. I've squashed all the patches together and have copied the result below. > From f7c27f204cab3d7dcaa128880c0d9ef1ae0e2fc6 Mon Sep 17 00:00:00 2001 > From: Lars-Peter Clausen > Date: Mon, 5 Mar 2012 16:30:57 +0100 > Subject: [PATCH] DRM: Add adv7511 encoder driver > > This patch adds a driver for the Analog Devices adv7511. The adv7511 is a > standalone HDMI transmitter chip. It features a HDMI output interface on one > end and video and audio input interfaces on the other. > > Signed-off-by: Lars-Peter Clausen > --- > drivers/gpu/drm/Kconfig | 6 + > drivers/gpu/drm/i2c/Makefile| 3 + > drivers/gpu/drm/i2c/adv7511.h | 454 + > drivers/gpu/drm/i2c/adv7511_audio.c | 304 +++ > drivers/gpu/drm/i2c/adv7511_core.c | 979 + > 5 files changed, 1746 insertions(+) > create mode 100644 drivers/gpu/drm/i2c/adv7511.h > create mode 100644 drivers/gpu/drm/i2c/adv7511_audio.c > create mode 100644 drivers/gpu/drm/i2c/adv7511_core.c First of all, please run checkpatch.pl on the code :-) > diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig > index 626bc0c..d8862a4 100644 > --- a/drivers/gpu/drm/Kconfig > +++ b/drivers/gpu/drm/Kconfig > @@ -200,6 +200,12 @@ config DRM_SAVAGE > Choose this option if you have a Savage3D/4/SuperSavage/Pro/Twister > chipset. If M is selected the module will be called savage. > > +config DRM_ENCODER_ADV7511 > + tristate "AV7511 encoder" > + depends on DRM && I2C > + select REGMAP_I2C > + select HDMI Beside adding a help text, you should also depend on and/or select SND_SOC. > + > source "drivers/gpu/drm/exynos/Kconfig" > > source "drivers/gpu/drm/vmwgfx/Kconfig" [snip] > diff --git a/drivers/gpu/drm/i2c/adv7511.h b/drivers/gpu/drm/i2c/adv7511.h > new file mode 100644 > index 000..4631fcd6 > --- /dev/null > +++ b/drivers/gpu/drm/i2c/adv7511.h > @@ -0,0 +1,454 @@ > +/** > + * Analog Devices ADV7511 HDMI transmitter driver > + * > + * Copyright 2012 Analog Devices Inc. > + * > + * Licensed under the GPL-2. > + */ > + > +#ifndef __ADV7511_H__ > +#define __ADV7511_H__ > + > +#include This file should be split in two headers, one with platform data that will go to include/linux/platform_data/, and another one that can stay here. > +#define ADV7511_REG_CHIP_REVISION0x00 > +#define ADV7511_REG_N0 0x01 > +#define ADV7511_REG_N1 0x02 > +#define ADV7511_REG_N2 0x03 > +#define ADV7511_REG_SPDIF_FREQ 0x04 > +#define ADV7511_REG_CTS_AUTOMATIC1 0x05 > +#
