Re: [PATCH 1/2] drm: Rename DP_PSR_SELECTIVE_UPDATE to better mach eDP spec
The changed name looks more accurate to the edp 1.4b spec. Looks good to me. Reviewed-by: Gwan-gyeong Mun On Wed, 2021-04-21 at 15:02 -0700, José Roberto de Souza wrote: > DP_PSR_EN_CFG bit 5 aka "Selective Update Region Scan Line Capture > Indication" in eDP spec has a ambiguous name, so renaming to better > match specification. > > While at it, replacing bit shit by BIT() macro and adding the version > some registers were added to eDP specification. > > Cc: > Cc: Rodrigo Vivi > Cc: Jani Nikula > Cc: Gwan-gyeong Mun > Signed-off-by: José Roberto de Souza > --- > include/drm/drm_dp_helper.h | 16 > 1 file changed, 8 insertions(+), 8 deletions(-) > > diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h > index 1e85c2021f2f..d6f6a084a190 100644 > --- a/include/drm/drm_dp_helper.h > +++ b/include/drm/drm_dp_helper.h > @@ -687,14 +687,14 @@ struct drm_device; > #define DP_DSC_ENABLE 0x160 /* DP 1.4 */ > # define DP_DECOMPRESSION_EN (1 << 0) > > -#define DP_PSR_EN_CFG 0x170 /* XXX 1.2? */ > -# define DP_PSR_ENABLE (1 << 0) > -# define DP_PSR_MAIN_LINK_ACTIVE (1 << 1) > -# define DP_PSR_CRC_VERIFICATION (1 << 2) > -# define DP_PSR_FRAME_CAPTURE (1 << 3) > -# define DP_PSR_SELECTIVE_UPDATE (1 << 4) > -# define DP_PSR_IRQ_HPD_WITH_CRC_ERRORS (1 << 5) > -# define DP_PSR_ENABLE_PSR2 (1 << 6) /* eDP 1.4a */ > +#define DP_PSR_EN_CFG 0x170 /* XXX 1.2? */ > +# define DP_PSR_ENABLE BIT(0) > +# define DP_PSR_MAIN_LINK_ACTIVE BIT(1) > +# define DP_PSR_CRC_VERIFICATION BIT(2) > +# define DP_PSR_FRAME_CAPTURE BIT(3) > +# define DP_PSR_SU_REGION_SCANLINE_CAPTURE BIT(4) /* eDP 1.4a */ > +# define DP_PSR_IRQ_HPD_WITH_CRC_ERRORSBIT(5) /* eDP > 1.4a */ > +# define DP_PSR_ENABLE_PSR2BIT(6) /* eDP 1.4a */ > > #define DP_ADAPTER_CTRL 0x1a0 > # define DP_ADAPTER_CTRL_FORCE_LOAD_SENSE (1 << 0) ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v9 1/5] drm: Add function to convert rect in 16.16 fixed format to regular format
On Fri, 2020-12-18 at 10:46 -0800, José Roberto de Souza wrote:
> Much more clear to read one function call than four lines doing this
> conversion.
>
> v7:
> - function renamed
> - calculating width and height before truncate
> - inlined
>
> Cc: Ville Syrjälä
> Cc: dri-devel@lists.freedesktop.org
> Cc: Gwan-gyeong Mun
> Signed-off-by: José Roberto de Souza
> ---
> include/drm/drm_rect.h | 13 +
> 1 file changed, 13 insertions(+)
>
> diff --git a/include/drm/drm_rect.h b/include/drm/drm_rect.h
> index e7f4d24cdd00..7eb84af4a818 100644
> --- a/include/drm/drm_rect.h
> +++ b/include/drm/drm_rect.h
> @@ -206,6 +206,19 @@ static inline bool drm_rect_equals(const struct
> drm_rect *r1,
> r1->y1 == r2->y1 && r1->y2 == r2->y2;
> }
>
> +/**
> + * drm_rect_fp_to_int - Convert a rect in 16.16 fixed point form to
> int form.
> + * @destination: rect to be stored the converted value
> + * @source: rect in 16.16 fixed point form
> + */
> +static inline void drm_rect_fp_to_int(struct drm_rect *destination,
> + const struct drm_rect *source)
other drm_rect functions use input parameter names as src, dst.
( ex. drm_rect_clip_scaled(), drm_rect_calc_hscale(),
drm_rect_calc_vscale() )
if the names change to src and dst, other parts seem good to me.
Reviewed-by: Gwan-gyeong Mun
> +{
> + drm_rect_init(destination, source->x1 >> 16, source->y1 >> 16,
> + drm_rect_width(source) >> 16,
> + drm_rect_height(source) >> 16);
> +}
> +
> bool drm_rect_intersect(struct drm_rect *r, const struct drm_rect
> *clip);
> bool drm_rect_clip_scaled(struct drm_rect *src, struct drm_rect
> *dst,
> const struct drm_rect *clip);
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v6 1/5] drm: Add function to convert rect in 16.16 fixed format to regular format
On Mon, 2020-12-14 at 09:49 -0800, José Roberto de Souza wrote:
> Much more clear to read one function call than four lines doing this
> conversion.
>
> Cc: dri-devel@lists.freedesktop.org
> Cc: Gwan-gyeong Mun
> Signed-off-by: José Roberto de Souza
> ---
> drivers/gpu/drm/drm_rect.c | 15 +++
> include/drm/drm_rect.h | 2 ++
> 2 files changed, 17 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_rect.c b/drivers/gpu/drm/drm_rect.c
> index 0460e874896e..24345704b353 100644
> --- a/drivers/gpu/drm/drm_rect.c
> +++ b/drivers/gpu/drm/drm_rect.c
> @@ -373,3 +373,18 @@ void drm_rect_rotate_inv(struct drm_rect *r,
> }
> }
> EXPORT_SYMBOL(drm_rect_rotate_inv);
> +
> +/**
> + * drm_rect_convert_16_16_to_regular - Convert a rect in 16.16 fixed
> point form
> + * to regular form.
> + * @in: rect in 16.16 fixed point form
> + * @out: rect to be stored the converted value
> + */
> +void drm_rect_convert_16_16_to_regular(struct drm_rect *in, struct
> drm_rect *out)
> +{
> + out->x1 = in->x1 >> 16;
> + out->y1 = in->y1 >> 16;
> + out->x2 = in->x2 >> 16;
> + out->y2 = in->y2 >> 16;
> +}
> +EXPORT_SYMBOL(drm_rect_convert_16_16_to_regular);
> diff --git a/include/drm/drm_rect.h b/include/drm/drm_rect.h
> index e7f4d24cdd00..2ef8180416cd 100644
> --- a/include/drm/drm_rect.h
> +++ b/include/drm/drm_rect.h
> @@ -223,5 +223,7 @@ void drm_rect_rotate(struct drm_rect *r,
> void drm_rect_rotate_inv(struct drm_rect *r,
>int width, int height,
>unsigned int rotation);
> +void drm_rect_convert_16_16_to_regular(struct drm_rect *in,
> +struct drm_rect *out);
>
Hi,
if it's purpose is just converting 16.16 fp to integer, how about you
have function prototype like this?
extern inline struct drm_rect
drm_rect_convert_16_16_fp_to_integer(struct drm_rect in)
And if there are no use case on drm core or other drivers except i915
display yet,
before adding this function to drm core, how about you add this
function code to i915 first?
Br,
G.G.
> #endif
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v5 1/6] drm/damage_helper: Check if damage clips has valid values
On Mon, 2020-12-14 at 08:55 +, Simon Ser wrote: > > Userspace can set a damage clip with a negative coordinate, > > negative > > width or height or larger than the plane. > > This invalid values could cause issues in some HW or even worst > > enable > > security flaws. > > > > v2: > > - add debug messages to let userspace know why atomic commit failed > > due invalid damage clips > > > > Cc: Simon Ser > > Cc: Gwan-gyeong Mun > > Cc: Sean Paul > > Cc: Fabio Estevam > > Cc: Deepak Rawat > > Cc: dri-devel@lists.freedesktop.org > > Signed-off-by: José Roberto de Souza > > After looking at the kernel code, it seems like the kernel already > checks for > all of that in drm_atomic_plane_check. Are you aware of this? > > > + w = drm_rect_width(_state->src) >> 16; > > + h = drm_rect_height(_state->src) >> 16; > > The docs say this should be in FB coordinates, not in SRC_* > coordinates. So we > shouldn't need to check any SRC_* prop here. > I agree the Simon's opinion. it does check between plane's frame buffer src geometry and damage clips. (Plane's damage clip might exist outside of fb src geometry.) ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v12 00/14] In order to readout DP SDPs, refactors the handling of DP SDPs
Hi Ville, Thank you for notifying me that. I definitely missed the crash. Sorry for that. Danial and Jani, I' under debugging the crash case. If you are availabe please do not merge current version. Br, G.G. > On Fri, 2020-05-15 at 16:14 +0200, Daniel Vetter wrote: > On Fri, May 15, 2020 at 04:13:18PM +0300, Jani Nikula wrote: > > On Fri, 15 May 2020, Ville Syrjälä > > wrote: > > > On Thu, May 14, 2020 at 02:19:23PM +0300, Jani Nikula wrote: > > > > On Thu, 14 May 2020, Gwan-gyeong Mun > > > > wrote: > > > > > In order to readout DP SDPs (Secondary Data Packet: DP HDR > > > > > Metadata > > > > > Infoframe SDP, DP VSC SDP), it refactors handling DP SDPs > > > > > codes. > > > > > It adds new compute routines for DP HDR Metadata Infoframe > > > > > SDP > > > > > and DP VSC SDP. > > > > > And new writing routines of DP SDPs (Secondary Data Packet) > > > > > that uses > > > > > computed configs. > > > > > New reading routines of DP SDPs are added for readout. > > > > > It adds a logging function for DP VSC SDP. > > > > > When receiving video it is very useful to be able to log DP > > > > > VSC SDP. > > > > > This greatly simplifies debugging. > > > > > In order to use a common VSC SDP Colorimetry calculating code > > > > > on PSR, > > > > > it uses a new psr vsc sdp compute routine. > > > > > > > > Pushed the series to drm-intel-next-queued with Daniel's irc > > > > ack for > > > > merging the two non-i915 patches that route too. > > > > > > fi-hsw-4770 now oopses at boot: > > > > /o\ > > > > What did I miss? What part about the CI report did I overlook? > > Participating hosts (48 -> 45) > -- > > Additional (1): fi-kbl-7560u > Missing(4): fi-byt-clapper fi-byt-squawks fi-bsw-cyan fi-hsw- > 4200u > > > You kill machines at boot, CI won't tell you. > > This is (or at least was) because the network is shitty enough that > we > have more spurious failures because the ethernet went into the ether > than > because of people having killed the machine with their patches for > real. > Also it's hard to grab logs if the thing doesn't work at all, so cant > give > you any more data than the above. > > Yes this sucks :-/ > > Cheers, Daniel > > > BR, > > Jani. > > > > > > > <1>[3.736903] BUG: kernel NULL pointer dereference, address: > > > > > > <1>[3.736916] #PF: supervisor read access in kernel mode > > > <1>[3.736916] #PF: error_code(0x) - not-present page > > > <6>[3.736917] PGD 0 P4D 0 > > > <4>[3.736919] Oops: [#1] PREEMPT SMP PTI > > > <4>[3.736921] CPU: 0 PID: 363 Comm: systemd-udevd Not tainted > > > 5.7.0-rc5-CI-CI_DRM_8485+ #1 > > > <4>[3.736922] Hardware name: LENOVO 10AGS00601/SHARKBAY, BIOS > > > FBKT34AUS 04/24/2013 > > > <4>[3.736986] RIP: 0010:intel_psr_enabled+0x8/0x70 [i915] > > > <4>[3.736988] Code: 18 48 c7 c6 40 09 79 a0 e8 e3 e2 04 e1 0f > > > b6 44 24 03 e9 f4 fd ff ff 90 66 2e 0f 1f 84 00 00 00 00 00 41 54 > > > 55 53 48 83 ec 08 <48> 8b 9f d8 fe ff ff f6 83 5e 0d 00 00 20 74 > > > 09 80 bb 6c b6 00 00 > > > <4>[3.737036] RSP: 0018:c947f8a0 EFLAGS: 00010286 > > > <4>[3.737042] RAX: 0002 RBX: 8883ffd04000 > > > RCX: 0001 > > > <4>[3.737048] RDX: 0007 RSI: 8883ffd04000 > > > RDI: 0128 > > > <4>[3.737055] RBP: 888406afe200 R08: 000f > > > R09: 0001 > > > <4>[3.737061] R10: R11: > > > R12: > > > <4>[3.737068] R13: 8883f75d R14: 888406afe200 > > > R15: 8883f75d0870 > > > <4>[3.737075] FS: 7f71618f9680() > > > GS:88840ec0() knlGS: > > > <4>[3.737082] CS: 0010 DS: ES: CR0: > > > 80050033 > > > <4>[3.737088] CR2: CR3: 000402510002 > > > CR4: 001606f0 > > > <4>[3.737094] DR0: DR1: > > > DR2: > > > <4>[3.737101] DR3: DR6: fffe0ff0 > > > DR7: 0400 > > > <4>[3.737107] Call Trace: > > > <4>[3.737175] intel_read_dp_sdp+0x1a4/0x380 [i915] > > > <4>[3.737246] hsw_crt_get_config+0x12/0x40 [i915] > > > <4>[3.737317] intel_modeset_setup_hw_state+0x3b3/0x16a0 > > > [i915] > > > ... > > > > -- > > Jani Nikula, Intel Open Source Graphics Center ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [Intel-gfx] [PATCH v12 01/14] video/hdmi: Add Unpack only function for DRM infoframe
Hi Bartlomiej and Laurent Pinchart, can I have your ack for merging
this via drm-intel along
with the rest of the series, please?
BR,
G.G.
On Thu, 2020-05-14 at 09:07 +0300, Gwan-gyeong Mun wrote:
> It adds an unpack only function for DRM infoframe for dynamic range
> and
> mastering infoframe readout.
> It unpacks the information data block contained in the binary buffer
> into
> a structured frame of the HDMI Dynamic Range and Mastering (DRM)
> information frame.
>
> In contrast to hdmi_drm_infoframe_unpack() function, it does not
> verify
> a checksum.
>
> It can be used for unpacking a DP HDR Metadata Infoframe SDP case.
> DP HDR Metadata Infoframe SDP uses the same Dynamic Range and
> Mastering
> (DRM) information (CTA-861-G spec.) such as HDMI DRM infoframe.
> But DP SDP header and payload structure are different from HDMI DRM
> Infoframe. Therefore unpacking DRM infoframe for DP requires skipping
> of
> a verifying checksum.
>
> v9: Add clear comments to hdmi_drm_infoframe_unpack_only() and
> hdmi_drm_infoframe_unpack() (Laurent Pinchart)
>
> Signed-off-by: Gwan-gyeong Mun
> Reviewed-by: Uma Shankar
> Cc: Laurent Pinchart
> Cc: Ville Syrjala
> ---
> drivers/video/hdmi.c | 65 +++---
> --
> include/linux/hdmi.h | 2 ++
> 2 files changed, 48 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/video/hdmi.c b/drivers/video/hdmi.c
> index 856a8c4e84a2..e70792b3e367 100644
> --- a/drivers/video/hdmi.c
> +++ b/drivers/video/hdmi.c
> @@ -1768,20 +1768,21 @@ hdmi_vendor_any_infoframe_unpack(union
> hdmi_vendor_any_infoframe *frame,
> }
>
> /**
> - * hdmi_drm_infoframe_unpack() - unpack binary buffer to a HDMI DRM
> infoframe
> + * hdmi_drm_infoframe_unpack_only() - unpack binary buffer of CTA-
> 861-G DRM
> + *infoframe DataBytes to a HDMI
> DRM
> + *infoframe
> * @frame: HDMI DRM infoframe
> * @buffer: source buffer
> * @size: size of buffer
> *
> - * Unpacks the information contained in binary @buffer into a
> structured
> - * @frame of the HDMI Dynamic Range and Mastering (DRM) information
> frame.
> - * Also verifies the checksum as required by section 5.3.5 of the
> HDMI 1.4
> - * specification.
> + * Unpacks CTA-861-G DRM infoframe DataBytes contained in the binary
> @buffer
> + * into a structured @frame of the HDMI Dynamic Range and Mastering
> (DRM)
> + * infoframe.
> *
> * Returns 0 on success or a negative error code on failure.
> */
> -static int hdmi_drm_infoframe_unpack(struct hdmi_drm_infoframe
> *frame,
> - const void *buffer, size_t size)
> +int hdmi_drm_infoframe_unpack_only(struct hdmi_drm_infoframe *frame,
> +const void *buffer, size_t size)
> {
> const u8 *ptr = buffer;
> const u8 *temp;
> @@ -1790,23 +1791,13 @@ static int hdmi_drm_infoframe_unpack(struct
> hdmi_drm_infoframe *frame,
> int ret;
> int i;
>
> - if (size < HDMI_INFOFRAME_SIZE(DRM))
> - return -EINVAL;
> -
> - if (ptr[0] != HDMI_INFOFRAME_TYPE_DRM ||
> - ptr[1] != 1 ||
> - ptr[2] != HDMI_DRM_INFOFRAME_SIZE)
> - return -EINVAL;
> -
> - if (hdmi_infoframe_checksum(buffer, HDMI_INFOFRAME_SIZE(DRM))
> != 0)
> + if (size < HDMI_DRM_INFOFRAME_SIZE)
> return -EINVAL;
>
> ret = hdmi_drm_infoframe_init(frame);
> if (ret)
> return ret;
>
> - ptr += HDMI_INFOFRAME_HEADER_SIZE;
> -
> frame->eotf = ptr[0] & 0x7;
> frame->metadata_type = ptr[1] & 0x7;
>
> @@ -1814,7 +1805,7 @@ static int hdmi_drm_infoframe_unpack(struct
> hdmi_drm_infoframe *frame,
> for (i = 0; i < 3; i++) {
> x_lsb = *temp++;
> x_msb = *temp++;
> - frame->display_primaries[i].x = (x_msb << 8) | x_lsb;
> + frame->display_primaries[i].x = (x_msb << 8) | x_lsb;
> y_lsb = *temp++;
> y_msb = *temp++;
> frame->display_primaries[i].y = (y_msb << 8) | y_lsb;
> @@ -1830,6 +1821,42 @@ static int hdmi_drm_infoframe_unpack(struct
> hdmi_drm_infoframe *frame,
>
> return 0;
> }
> +EXPORT_SYMBOL(hdmi_drm_infoframe_unpack_only);
> +
> +/**
> + * hdmi_drm_infoframe_unpack() - unpack binary buffer to a HDMI DRM
> infoframe
> + * @frame: HDMI DRM infoframe
> + * @buffer: source buffer
> + * @size: size of buffer
> + *
> + * Unpacks the CTA-861-G DRM infoframe contained in the binary
> @buffer into
> + * a structured @frame of the HDMI Dynamic Range and Mastering (DRM)
> + * infoframe. It also verifies the checksum as required by section
> 5.3.5 of
> + * the HDMI 1.4 specification.
> + *
> + * Returns 0 on success or a negative error code on failure.
> + */
> +static int hdmi_drm_infoframe_unpack(struct hdmi_drm_infoframe
> *frame,
> + const void *buffer, size_t size)
> +{
> + const u8 *ptr =
Re: [Intel-gfx] [PATCH v7 05/18] video/hdmi: Add Unpack only function for DRM infoframe
On Fri, 2020-03-27 at 14:56 +0200, Ville Syrjälä wrote: > On Fri, Mar 27, 2020 at 07:27:56AM +0000, Mun, Gwan-gyeong wrote: > > On Fri, 2020-03-20 at 13:57 +0200, Laurent Pinchart wrote: > > > Hi Jani, > > > > > > On Fri, Mar 20, 2020 at 01:32:17PM +0200, Jani Nikula wrote: > > > > On Fri, 20 Mar 2020, Jani Nikula > > > > wrote: > > > > > On Tue, 11 Feb 2020, Gwan-gyeong Mun < > > > > > gwan-gyeong@intel.com> > > > > > wrote: > > > > > > It adds an unpack only function for DRM infoframe for > > > > > > dynamic > > > > > > range and > > > > > > mastering infoframe readout. > > > > > > It unpacks the information data block contained in the > > > > > > binary > > > > > > buffer into > > > > > > a structured frame of the HDMI Dynamic Range and Mastering > > > > > > (DRM) > > > > > > information frame. > > > > > > > > > > > > In contrast to hdmi_drm_infoframe_unpack() function, it > > > > > > does > > > > > > not verify > > > > > > a checksum. > > > > > > > > > > > > It can be used for unpacking a DP HDR Metadata Infoframe > > > > > > SDP > > > > > > case. > > > > > > DP HDR Metadata Infoframe SDP uses the same Dynamic Range > > > > > > and > > > > > > Mastering > > > > > > (DRM) information (CTA-861-G spec.) such as HDMI DRM > > > > > > infoframe. > > > > > > But DP SDP header and payload structure are different from > > > > > > HDMI > > > > > > DRM > > > > > > Infoframe. Therefore unpacking DRM infoframe for DP > > > > > > requires > > > > > > skipping of > > > > > > a verifying checksum. > > > > > > > > > > > > Signed-off-by: Gwan-gyeong Mun > > > > > > Reviewed-by: Uma Shankar > > > > > > > > > > Bartlomiej, can I have your ack for merging this via drm- > > > > > intel > > > > > along > > > > > with the rest of the series, please? > > > > > > > > Or Hans or Laurent, from v4l/video point of view. > > > > > > I'm no expert on InfoFrame, I'll only comment on the API below. > > > > > > > > > --- > > > > > > drivers/video/hdmi.c | 58 +++- > > > > > > > > > > > > > > > > > > include/linux/hdmi.h | 2 ++ > > > > > > 2 files changed, 43 insertions(+), 17 deletions(-) > > > > > > > > > > > > diff --git a/drivers/video/hdmi.c b/drivers/video/hdmi.c > > > > > > index 9c82e2a0a411..9818836d82b7 100644 > > > > > > --- a/drivers/video/hdmi.c > > > > > > +++ b/drivers/video/hdmi.c > > > > > > @@ -1775,20 +1775,18 @@ > > > > > > hdmi_vendor_any_infoframe_unpack(union > > > > > > hdmi_vendor_any_infoframe *frame, > > > > > > } > > > > > > > > > > > > /** > > > > > > - * hdmi_drm_infoframe_unpack() - unpack binary buffer to a > > > > > > HDMI DRM infoframe > > > > > > + * hdmi_drm_infoframe_unpack_only() - unpack binary buffer > > > > > > to > > > > > > a HDMI DRM infoframe > > > > > > * @frame: HDMI DRM infoframe > > > > > > * @buffer: source buffer > > > > > > * @size: size of buffer > > > > > > * > > > > > > - * Unpacks the information contained in binary @buffer > > > > > > into a > > > > > > structured > > > > > > + * Unpacks the information data block contained in binary > > > > > > @buffer into a structured > > > > > > Line wrap please. > > > > > > This needs to be clarified to explain exactly what the buffer > > > points > > > to. > > > > > Okay I'll update clear comments next version. > > > Also, as this is applicable to DP too, shouldn't we drop the > > > hdmi_ > > > prefix ? Is there another prefix that could be used for functions > > > that > > > are application to infoframe handling shared by different display > > > interfaces ? A bit of refactoring would help making all this > > > clear. > > > > > Both DP and HDMI use CTA-861-G spec for DRM infoframe. I'll update > > prefix with cta_ instead of hdmi_. > > Most of video/hdmi.c is from the CTA spec(s). The name is just a > name. > Let's not start making it inconsistent just for this one case. > Hi Ville, thank you for giving me your opinion. And I agree with your opinion. I'll update detailed comments with consistent API namings. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [Intel-gfx] [PATCH v7 05/18] video/hdmi: Add Unpack only function for DRM infoframe
On Fri, 2020-03-20 at 13:57 +0200, Laurent Pinchart wrote:
> Hi Jani,
>
> On Fri, Mar 20, 2020 at 01:32:17PM +0200, Jani Nikula wrote:
> > On Fri, 20 Mar 2020, Jani Nikula
> > wrote:
> > > On Tue, 11 Feb 2020, Gwan-gyeong Mun
> > > wrote:
> > > > It adds an unpack only function for DRM infoframe for dynamic
> > > > range and
> > > > mastering infoframe readout.
> > > > It unpacks the information data block contained in the binary
> > > > buffer into
> > > > a structured frame of the HDMI Dynamic Range and Mastering
> > > > (DRM)
> > > > information frame.
> > > >
> > > > In contrast to hdmi_drm_infoframe_unpack() function, it does
> > > > not verify
> > > > a checksum.
> > > >
> > > > It can be used for unpacking a DP HDR Metadata Infoframe SDP
> > > > case.
> > > > DP HDR Metadata Infoframe SDP uses the same Dynamic Range and
> > > > Mastering
> > > > (DRM) information (CTA-861-G spec.) such as HDMI DRM infoframe.
> > > > But DP SDP header and payload structure are different from HDMI
> > > > DRM
> > > > Infoframe. Therefore unpacking DRM infoframe for DP requires
> > > > skipping of
> > > > a verifying checksum.
> > > >
> > > > Signed-off-by: Gwan-gyeong Mun
> > > > Reviewed-by: Uma Shankar
> > >
> > > Bartlomiej, can I have your ack for merging this via drm-intel
> > > along
> > > with the rest of the series, please?
> >
> > Or Hans or Laurent, from v4l/video point of view.
>
> I'm no expert on InfoFrame, I'll only comment on the API below.
>
> > > > ---
> > > > drivers/video/hdmi.c | 58 +++-
> > > >
> > > > include/linux/hdmi.h | 2 ++
> > > > 2 files changed, 43 insertions(+), 17 deletions(-)
> > > >
> > > > diff --git a/drivers/video/hdmi.c b/drivers/video/hdmi.c
> > > > index 9c82e2a0a411..9818836d82b7 100644
> > > > --- a/drivers/video/hdmi.c
> > > > +++ b/drivers/video/hdmi.c
> > > > @@ -1775,20 +1775,18 @@ hdmi_vendor_any_infoframe_unpack(union
> > > > hdmi_vendor_any_infoframe *frame,
> > > > }
> > > >
> > > > /**
> > > > - * hdmi_drm_infoframe_unpack() - unpack binary buffer to a
> > > > HDMI DRM infoframe
> > > > + * hdmi_drm_infoframe_unpack_only() - unpack binary buffer to
> > > > a HDMI DRM infoframe
> > > > * @frame: HDMI DRM infoframe
> > > > * @buffer: source buffer
> > > > * @size: size of buffer
> > > > *
> > > > - * Unpacks the information contained in binary @buffer into a
> > > > structured
> > > > + * Unpacks the information data block contained in binary
> > > > @buffer into a structured
>
> Line wrap please.
>
> This needs to be clarified to explain exactly what the buffer points
> to.
>
Okay I'll update clear comments next version.
> Also, as this is applicable to DP too, shouldn't we drop the hdmi_
> prefix ? Is there another prefix that could be used for functions
> that
> are application to infoframe handling shared by different display
> interfaces ? A bit of refactoring would help making all this clear.
>
Both DP and HDMI use CTA-861-G spec for DRM infoframe. I'll update
prefix with cta_ instead of hdmi_.
> > > > * @frame of the HDMI Dynamic Range and Mastering (DRM)
> > > > information frame.
> > > > - * Also verifies the checksum as required by section 5.3.5 of
> > > > the HDMI 1.4
> > > > - * specification.
> > > > *
> > > > * Returns 0 on success or a negative error code on failure.
> > > > */
> > > > -static int hdmi_drm_infoframe_unpack(struct hdmi_drm_infoframe
> > > > *frame,
> > > > -const void *buffer, size_t
> > > > size)
> > > > +int hdmi_drm_infoframe_unpack_only(struct hdmi_drm_infoframe
> > > > *frame,
> > > > + const void *buffer, size_t
> > > > size)
> > > > {
> > > > const u8 *ptr = buffer;
> > > > const u8 *temp;
> > > > @@ -1797,23 +1795,13 @@ static int
> > > > hdmi_drm_infoframe_unpack(struct hdmi_drm_infoframe *frame,
> > > > int ret;
> > > > int i;
> > > >
> > > > - if (size < HDMI_INFOFRAME_SIZE(DRM))
> > > > - return -EINVAL;
> > > > -
> > > > - if (ptr[0] != HDMI_INFOFRAME_TYPE_DRM ||
> > > > - ptr[1] != 1 ||
> > > > - ptr[2] != HDMI_DRM_INFOFRAME_SIZE)
> > > > - return -EINVAL;
> > > > -
> > > > - if (hdmi_infoframe_checksum(buffer,
> > > > HDMI_INFOFRAME_SIZE(DRM)) != 0)
> > > > + if (size < HDMI_DRM_INFOFRAME_SIZE)
> > > > return -EINVAL;
> > > >
> > > > ret = hdmi_drm_infoframe_init(frame);
> > > > if (ret)
> > > > return ret;
> > > >
> > > > - ptr += HDMI_INFOFRAME_HEADER_SIZE;
> > > > -
> > > > frame->eotf = ptr[0] & 0x7;
> > > > frame->metadata_type = ptr[1] & 0x7;
> > > >
> > > > @@ -1837,6 +1825,42 @@ static int
> > > > hdmi_drm_infoframe_unpack(struct hdmi_drm_infoframe *frame,
> > > >
> > > > return 0;
> > > > }
> > > > +EXPORT_SYMBOL(hdmi_drm_infoframe_unpack_only);
> > > > +
> > > > +/**
> > > >
Re: [Intel-gfx] [PATCH v3 11/17] drm/i915: Program DP SDPs with computed configs
On Wed, 2020-02-05 at 22:21 +0530, Shankar, Uma wrote:
> > -Original Message-
> > From: Intel-gfx On Behalf
> > Of Gwan-
> > gyeong Mun
> > Sent: Tuesday, February 4, 2020 4:50 AM
> > To: intel-...@lists.freedesktop.org
> > Cc: linux-fb...@vger.kernel.org; dri-devel@lists.freedesktop.org
> > Subject: [Intel-gfx] [PATCH v3 11/17] drm/i915: Program DP SDPs
> > with computed
> > configs
> >
> > In order to use computed config for DP SDPs (DP VSC SDP and DP HDR
> > Metadata
> > Infoframe SDP), it replaces intel_dp_vsc_enable() function and
> > intel_dp_hdr_metadata_enable() function to
> > intel_dp_set_infoframes() function.
> >
> > Before applying it, routines of program SDP always calculated
> > configs when they
> > called. And it removes unused functions.
>
> Fix the sentence, seems unclear.
> With that fixed,
Okay, I'll update with the condition of before / after.
> Reviewed-by: Uma Shankar
>
> > v3: Rebased
> >
> > Signed-off-by: Gwan-gyeong Mun
> > ---
> > drivers/gpu/drm/i915/display/intel_ddi.c | 3 +-
> > drivers/gpu/drm/i915/display/intel_dp.c | 226 -
> > --
> > drivers/gpu/drm/i915/display/intel_dp.h | 6 -
> > 3 files changed, 1 insertion(+), 234 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c
> > b/drivers/gpu/drm/i915/display/intel_ddi.c
> > index c96f629cddc3..374ab6a3757c 100644
> > --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> > @@ -3900,8 +3900,7 @@ static void intel_enable_ddi_dp(struct
> > intel_encoder
> > *encoder,
> >
> > intel_edp_backlight_on(crtc_state, conn_state);
> > intel_psr_enable(intel_dp, crtc_state);
> > - intel_dp_vsc_enable(intel_dp, crtc_state, conn_state);
> > - intel_dp_hdr_metadata_enable(intel_dp, crtc_state, conn_state);
> > + intel_dp_set_infoframes(encoder, true, crtc_state, conn_state);
> > intel_edp_drrs_enable(intel_dp, crtc_state);
> >
> > if (crtc_state->has_audio)
> > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
> > b/drivers/gpu/drm/i915/display/intel_dp.c
> > index d4ece0a824c0..cffb77daec96 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > @@ -5095,232 +5095,6 @@ void intel_read_dp_sdp(struct intel_encoder
> > *encoder,
> > }
> > }
> >
> > -static void
> > -intel_dp_setup_vsc_sdp(struct intel_dp *intel_dp,
> > - const struct intel_crtc_state *crtc_state,
> > - const struct drm_connector_state *conn_state)
> > -{
> > - struct intel_digital_port *intel_dig_port =
> > dp_to_dig_port(intel_dp);
> > - struct dp_sdp vsc_sdp = {};
> > -
> > - /* Prepare VSC Header for SU as per DP 1.4a spec, Table 2-119
> > */
> > - vsc_sdp.sdp_header.HB0 = 0;
> > - vsc_sdp.sdp_header.HB1 = 0x7;
> > -
> > - /*
> > -* VSC SDP supporting 3D stereo, PSR2, and Pixel Encoding/
> > -* Colorimetry Format indication.
> > -*/
> > - vsc_sdp.sdp_header.HB2 = 0x5;
> > -
> > - /*
> > -* VSC SDP supporting 3D stereo, + PSR2, + Pixel Encoding/
> > -* Colorimetry Format indication (HB2 = 05h).
> > -*/
> > - vsc_sdp.sdp_header.HB3 = 0x13;
> > -
> > - /* DP 1.4a spec, Table 2-120 */
> > - switch (crtc_state->output_format) {
> > - case INTEL_OUTPUT_FORMAT_YCBCR444:
> > - vsc_sdp.db[16] = 0x1 << 4; /* YCbCr 444 : DB16[7:4] =
> > 1h */
> > - break;
> > - case INTEL_OUTPUT_FORMAT_YCBCR420:
> > - vsc_sdp.db[16] = 0x3 << 4; /* YCbCr 420 : DB16[7:4] =
> > 3h */
> > - break;
> > - case INTEL_OUTPUT_FORMAT_RGB:
> > - default:
> > - /* RGB: DB16[7:4] = 0h */
> > - break;
> > - }
> > -
> > - switch (conn_state->colorspace) {
> > - case DRM_MODE_COLORIMETRY_BT709_YCC:
> > - vsc_sdp.db[16] |= 0x1;
> > - break;
> > - case DRM_MODE_COLORIMETRY_XVYCC_601:
> > - vsc_sdp.db[16] |= 0x2;
> > - break;
> > - case DRM_MODE_COLORIMETRY_XVYCC_709:
> > - vsc_sdp.db[16] |= 0x3;
> > - break;
> > - case DRM_MODE_COLORIMETRY_SYCC_601:
> > - vsc_sdp.db[16] |= 0x4;
> > - break;
> > - case DRM_MODE_COLORIMETRY_OPYCC_601:
> > - vsc_sdp.db[16] |= 0x5;
> > - break;
> > - case DRM_MODE_COLORIMETRY_BT2020_CYCC:
> > - case DRM_MODE_COLORIMETRY_BT2020_RGB:
> > - vsc_sdp.db[16] |= 0x6;
> > - break;
> > - case DRM_MODE_COLORIMETRY_BT2020_YCC:
> > - vsc_sdp.db[16] |= 0x7;
> > - break;
> > - case DRM_MODE_COLORIMETRY_DCI_P3_RGB_D65:
> > - case DRM_MODE_COLORIMETRY_DCI_P3_RGB_THEATER:
> > - vsc_sdp.db[16] |= 0x4; /* DCI-P3 (SMPTE RP 431-2) */
> > - break;
> > - default:
> > - /* sRGB (IEC 61966-2-1) / ITU-R BT.601: DB16[0:3] = 0h
> > */
> > -
> > - /* RGB->YCBCR color conversion uses the BT.709 color
> > space. */
> > - if (crtc_state->output_format ==
> >
Re: [PATCH v3 06/17] drm/i915/dp: Read out DP SDPs (Secondary Data Packet)
On Wed, 2020-02-05 at 21:59 +0530, Shankar, Uma wrote:
> > -Original Message-
> > From: dri-devel On Behalf
> > Of Gwan-
> > gyeong Mun
> > Sent: Tuesday, February 4, 2020 4:50 AM
> > To: intel-...@lists.freedesktop.org
> > Cc: linux-fb...@vger.kernel.org; dri-devel@lists.freedesktop.org
> > Subject: [PATCH v3 06/17] drm/i915/dp: Read out DP SDPs (Secondary
> > Data Packet)
>
> Drop the content in bracket.
>
> > It adds code to read the DP SDPs from the video DIP and unpack them
> > into the crtc
> > state.
> >
> > It adds routines that read out DP VSC SDP and DP HDR Metadata
> > Infoframe SDP In
> > order to unpack DP VSC SDP, it adds intel_dp_vsc_sdp_unpack()
> > function.
> > It follows DP 1.4a spec. [Table 2-116: VSC SDP Header Bytes] and
> > [Table 2-117: VSC
> > SDP Payload for DB16 through DB18]
> >
> > In order to unpack DP HDR Metadata Infoframe SDP, it adds
> > intel_dp_hdr_metadata_infoframe_sdp_unpack(). And it follows DP
> > 1.4a spec.
> > ([Table 2-125: INFOFRAME SDP v1.2 Header Bytes] and [Table 2-126:
> > INFOFRAME
> > SDP v1.2 Payload Data Bytes - DB0 through DB31]) and CTA-861-G
> > spec. [Table-42
> > Dynamic Range and Mastering InfoFrame].
> >
> > A nameing rule and style of intel_read_dp_sdp() function references
>
> Typo in naming.
>
> > intel_read_infoframe() function of intel_hdmi.c
> >
> > v2: Minor style fix
> > v3: Replace a structure name to drm_dp_vsc_sdp from
> > intel_dp_vsc_sdp
> >
> > Signed-off-by: Gwan-gyeong Mun
> > ---
> > drivers/gpu/drm/i915/display/intel_dp.c | 170
> >
> > drivers/gpu/drm/i915/display/intel_dp.h | 3 +
> > 2 files changed, 173 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
> > b/drivers/gpu/drm/i915/display/intel_dp.c
> > index dd7e5588001e..d4ece0a824c0 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > @@ -4925,6 +4925,176 @@ void intel_dp_set_infoframes(struct
> > intel_encoder
> > *encoder,
> > intel_write_dp_sdp(encoder, crtc_state,
> > HDMI_PACKET_TYPE_GAMUT_METADATA); }
> >
> > +static int intel_dp_vsc_sdp_unpack(struct drm_dp_vsc_sdp *vsc,
> > + const void *buffer, size_t size) {
> > + const struct dp_sdp *sdp = buffer;
> > +
> > + if (size < sizeof(struct dp_sdp))
> > + return -EINVAL;
> > +
> > + memset(vsc, 0, size);
> > +
> > + if (sdp->sdp_header.HB0 != 0)
> > + return -EINVAL;
> > +
> > + if (sdp->sdp_header.HB1 != DP_SDP_VSC)
> > + return -EINVAL;
> > + vsc->sdp_type = sdp->sdp_header.HB1;
> > +
> > + if (sdp->sdp_header.HB2 == 0x2 && sdp->sdp_header.HB3 == 0x8) {
> > + vsc->revision = sdp->sdp_header.HB2;
> > + vsc->length = sdp->sdp_header.HB3;
> > + } else if (sdp->sdp_header.HB2 == 0x4 && sdp->sdp_header.HB3 ==
> > 0xe) {
> > + vsc->revision = sdp->sdp_header.HB2;
> > + vsc->length = sdp->sdp_header.HB3;
> > + } else if (sdp->sdp_header.HB2 == 0x5 && sdp->sdp_header.HB3 ==
> > 0x13) {
> > + vsc->revision = sdp->sdp_header.HB2;
> > + vsc->length = sdp->sdp_header.HB3;
>
> The above 2 lines can be done unconditionally, may be combine the if
> checks.
>
> > + vsc->colorspace = (sdp->db[16] >> 4) & 0xf;
> > + vsc->colorimetry = sdp->db[16] & 0xf;
> > + vsc->dynamic_range = (sdp->db[17] >> 7) & 0x1;
> > +
> > + switch (sdp->db[17] & 0x7) {
> > + case 0x1:
> > + vsc->bpc = 8;
> > + break;
> > + case 0x2:
> > + vsc->bpc = 10;
> > + break;
> > + case 0x3:
> > + vsc->bpc = 12;
> > + break;
> > + case 0x4:
> > + vsc->bpc = 16;
> > + break;
> > + default:
> > + MISSING_CASE(sdp->db[17] & 0x7);
>
> Handle 6bpc case as well.
>
Yes, I'll update everything that you commented.
> > + return -EINVAL;
> > + }
> > +
> > + vsc->content_type = sdp->db[18] & 0x7;
> > + } else {
> > + return -EINVAL;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int
> > +intel_dp_hdr_metadata_infoframe_sdp_unpack(struct
> > hdmi_drm_infoframe
> > *drm_infoframe,
> > + const void *buffer, size_t
> > size) {
> > + int ret;
> > +
> > + const struct dp_sdp *sdp = buffer;
> > +
> > + if (size < sizeof(struct dp_sdp))
> > + return -EINVAL;
> > +
> > + if (sdp->sdp_header.HB0 != 0)
> > + return -EINVAL;
> > +
> > + if (sdp->sdp_header.HB1 != HDMI_INFOFRAME_TYPE_DRM)
> > + return -EINVAL;
> > +
> > + /*
> > +* Least Significant Eight Bits of (Data Byte Count – 1)
> > +* 1Dh (i.e., Data Byte Count = 30 bytes).
> > +*/
> > + if (sdp->sdp_header.HB2 != 0x1D)
> > + return -EINVAL;
> > +
> > +
Re: [PATCH v3 04/17] drm/i915/dp: Add writing of DP SDPs (Secondary Data Packet)
On Wed, 2020-02-05 at 21:39 +0530, Shankar, Uma wrote:
> > -Original Message-
> > From: dri-devel On Behalf
> > Of Gwan-
> > gyeong Mun
> > Sent: Tuesday, February 4, 2020 4:50 AM
> > To: intel-...@lists.freedesktop.org
> > Cc: linux-fb...@vger.kernel.org; dri-devel@lists.freedesktop.org
> > Subject: [PATCH v3 04/17] drm/i915/dp: Add writing of DP SDPs
> > (Secondary Data
> > Packet)
>
> Drop things in (), not needed.
>
> > It adds routines that write DP VSC SDP and DP HDR Metadata
> > Infoframe SDP.
> > In order to pack DP VSC SDP, it adds intel_dp_vsc_sdp_pack()
> > function.
> > It follows DP 1.4a spec. [Table 2-116: VSC SDP Header Bytes] and
> > [Table 2-117: VSC
> > SDP Payload for DB16 through DB18]
> >
> > In order to pack DP HDR Metadata Infoframe SDP, it adds
> > intel_dp_hdr_metadata_infoframe_sdp_pack() function.
> > And it follows DP 1.4a spec.
> > ([Table 2-125: INFOFRAME SDP v1.2 Header Bytes] and [Table 2-126:
> > INFOFRAME
> > SDP v1.2 Payload Data Bytes - DB0 through DB31]) and CTA-861-G
> > spec. [Table-42
> > Dynamic Range and Mastering InfoFrame].
> >
> > A machanism and a naming rule of intel_dp_set_infoframes() function
> > references
>
> Typo in mechanism.
>
> > intel_encoder->set_infoframes() of intel_hdmi.c .
> > VSC SDP is used for PSR and Pixel Encoding and Colorimetry Formats
> > cases.
> > Because PSR routine has its own routine of writing a VSC SDP, when
> > the PSR is
> > enabled, intel_dp_set_infoframes() does not write a VSC SDP.
> >
> > v3:
> > - Explicitly disable unused DIPs (AVI, GCP, VS, SPD, DRM. They
> > will be
> > used for HDMI), when intel_dp_set_infoframes() function will be
> > called.
> > - Replace a structure name to drm_dp_vsc_sdp from
> > intel_dp_vsc_sdp.
> >
> > Signed-off-by: Gwan-gyeong Mun
> > ---
> > drivers/gpu/drm/i915/display/intel_dp.c | 194
> >
> > drivers/gpu/drm/i915/display/intel_dp.h | 3 +
> > 2 files changed, 197 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
> > b/drivers/gpu/drm/i915/display/intel_dp.c
> > index b265b5c599f2..dd7e5588001e 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > @@ -4731,6 +4731,200 @@ intel_dp_needs_vsc_sdp(const struct
> > intel_crtc_state
> > *crtc_state,
> > return false;
> > }
> >
> > +static ssize_t intel_dp_vsc_sdp_pack(const struct drm_dp_vsc_sdp
> > *vsc,
> > +struct dp_sdp *sdp, size_t size) {
> > + size_t length = sizeof(struct dp_sdp);
> > +
> > + if (size < length)
> > + return -ENOSPC;
> > +
> > + memset(sdp, 0, size);
> > +
> > + /*
> > +* Prepare VSC Header for SU as per DP 1.4a spec, Table 2-119
> > +* VSC SDP Header Bytes
> > +*/
> > + sdp->sdp_header.HB0 = 0; /* Secondary-Data Packet ID = 0 */
> > + sdp->sdp_header.HB1 = vsc->sdp_type; /* Secondary-data Packet
> > Type */
> > + sdp->sdp_header.HB2 = vsc->revision; /* Revision Number */
> > + sdp->sdp_header.HB3 = vsc->length; /* Number of Valid Data
> > Bytes */
> > +
> > + /* VSC SDP Payload for DB16 through DB18 */
> > + /* Pixel Encoding and Colorimetry Formats */
> > + sdp->db[16] = (vsc->colorspace & 0xf) << 4; /* DB16[7:4] */
> > + sdp->db[16] |= vsc->colorimetry & 0xf; /* DB16[3:0] */
> > +
> > + switch (vsc->bpc) {
> > + case 8:
> > + sdp->db[17] = 0x1; /* DB17[3:0] */
> > + break;
> > + case 10:
> > + sdp->db[17] = 0x2;
> > + break;
> > + case 12:
> > + sdp->db[17] = 0x3;
> > + break;
> > + case 16:
> > + sdp->db[17] = 0x4;
> > + break;
> > + default:
> > + MISSING_CASE(vsc->bpc);
>
> 6bpc is not handled here, add that as well.
>
Yes, I missed 6bpc case, I'll update it.
> > + break;
> > + }
> > + /* Dynamic Range and Component Bit Depth */
> > + if (vsc->dynamic_range == DP_DYNAMIC_RANGE_CTA)
> > + sdp->db[17] |= 0x80; /* DB17[7] */
> > +
> > + /* Content Type */
> > + sdp->db[18] = vsc->content_type & 0x7;
> > +
> > + return length;
> > +}
> > +
> > +static ssize_t
> > +intel_dp_hdr_metadata_infoframe_sdp_pack(const struct
> > hdmi_drm_infoframe
> > *drm_infoframe,
> > +struct dp_sdp *sdp,
> > +size_t size)
> > +{
> > + size_t length = sizeof(struct dp_sdp);
> > + const int infoframe_size = HDMI_INFOFRAME_HEADER_SIZE +
> > HDMI_DRM_INFOFRAME_SIZE;
> > + unsigned char buf[HDMI_INFOFRAME_HEADER_SIZE +
> > HDMI_DRM_INFOFRAME_SIZE];
> > + ssize_t len;
> > +
> > + if (size < length)
> > + return -ENOSPC;
> > +
> > + memset(sdp, 0, size);
> > +
> > + len = hdmi_drm_infoframe_pack_only(drm_infoframe, buf,
> > sizeof(buf));
> > + if (len < 0) {
> > + DRM_DEBUG_KMS("buffer size is smaller than hdr metadata
> > infoframe\n");
> > + return -ENOSPC;
> > + }
> > +
> > +
Re: [PATCH v3 01/17] drm: add DP 1.4 VSC SDP Payload related enums and a structure
On Wed, 2020-02-05 at 20:12 +0530, Shankar, Uma wrote:
> > -Original Message-
> > From: dri-devel On Behalf
> > Of Gwan-
> > gyeong Mun
> > Sent: Tuesday, February 4, 2020 4:50 AM
> > To: intel-...@lists.freedesktop.org
> > Cc: linux-fb...@vger.kernel.org; dri-devel@lists.freedesktop.org
> > Subject: [PATCH v3 01/17] drm: add DP 1.4 VSC SDP Payload related
> > enums and a
> > structure
>
> %s/add/Add/
> Also you can rephrase this as " Add DP1.4 VSC SDP Payload related
> Data Structures"/
>
Hi Uma,
Thank you for reviewing a patch series.
Okay I'll rephrase commit message with your guide.
> > It adds new enumeration definitions for VSC SDP Payload for Pixel
> > Encoding/Colorimetry Format.
> > And it adds a new drm data structure for DP VSC SDP.
> >
> > enum dp_colorspace and enum dp_colorimetry correspond "Pixel
> > Encoding and
> > Colorimetry Formats". enum dp_dynamic_range corresponds "Dynamic
> > Range".
> > And enum dp_content_type corresponds "Content Type"
> > All of them are based on DP 1.4 spec [Table 2-117: VSC SDP Payload
> > for
> > DB16 through DB18].
> >
> > v3: Add a new drm data structure for DP VSC SDP
> >
> > Signed-off-by: Gwan-gyeong Mun
> > ---
> > include/drm/drm_dp_helper.h | 57
> > +
> > 1 file changed, 57 insertions(+)
> >
> > diff --git a/include/drm/drm_dp_helper.h
> > b/include/drm/drm_dp_helper.h index
> > 262faf9e5e94..c098727681fa 100644
> > --- a/include/drm/drm_dp_helper.h
> > +++ b/include/drm/drm_dp_helper.h
> > @@ -1209,6 +1209,63 @@ struct dp_sdp {
> > #define EDP_VSC_PSR_UPDATE_RFB (1<<1)
> > #define EDP_VSC_PSR_CRC_VALUES_VALID (1<<2)
> >
> > +/* Based on DP 1.4 spec [Table 2-117: VSC SDP Payload for DB16
> > through
> > +DB18] */ enum dp_colorspace {
>
> We should not call this colorspace, rather rename it to
> dp_pixelformat.
> These are not colorspaces.
>
> Also add these to kernel docs as they are standard definitions.
>
> > + DP_COLORSPACE_RGB = 0,
>
> Make this as DP_PIXELFORMAT_RGB and rename all the below ones as
> well.
>
> > + DP_COLORSPACE_YUV444 = 0x1,
> > + DP_COLORSPACE_YUV422 = 0x2,
> > + DP_COLORSPACE_YUV420 = 0x3,
> > + DP_COLORSPACE_Y_ONLY = 0x4,
> > + DP_COLORSPACE_RAW = 0x5,
> > + DP_COLORSPACE_RESERVED = 0x6,
> > +};
> > +
> > +/**
> > + * Based on DP 1.4 spec [Table 2-117: VSC SDP Payload for DB16
> > through
> > +DB18]
> > + * and a name of enum member followes DRM_MODE_COLORIMETRY
> > definition.
>
> Typo in follows
>
> > + */
> > +enum dp_colorimetry {
>
> You can call this as dp_colorspace (this is actual colorspace), you
> can stick with colorimetry as well.
> Will leave that to you.
Yes, the DP spec uses colorimetry as the term, I prefer colorimetry.
>
> > + DP_COLORIMETRY_DEFAULT = 0, /* sRGB (IEC 61966-2-1) / ITU-R
> > BT.601 */
> > + DP_COLORIMETRY_RGB_WIDE_FIXED = 0x1,
> > + DP_COLORIMETRY_BT709_YCC = 0x1,
> > + DP_COLORIMETRY_RGB_WIDE_FLOAT = 0x2,
> > + DP_COLORIMETRY_XVYCC_601 = 0x2,
> > + DP_COLORIMETRY_OPRGB = 0x3,
> > + DP_COLORIMETRY_XVYCC_709 = 0x3,
> > + DP_COLORIMETRY_DCI_P3_RGB = 0x4,
> > + DP_COLORIMETRY_SYCC_601 = 0x4,
> > + DP_COLORIMETRY_RGB_CUSTOM = 0x5,
> > + DP_COLORIMETRY_OPYCC_601 = 0x5,
> > + DP_COLORIMETRY_BT2020_RGB = 0x6,
> > + DP_COLORIMETRY_BT2020_CYCC = 0x6,
> > + DP_COLORIMETRY_BT2020_YCC = 0x7,
> > +};
> > +
> > +enum dp_dynamic_range {
> > + DP_DYNAMIC_RANGE_VESA = 0,
> > + DP_DYNAMIC_RANGE_CTA = 1,
> > +};
> > +
> > +enum dp_content_type {
> > + DP_CONTENT_TYPE_NOT_DEFINED = 0x00,
> > + DP_CONTENT_TYPE_GRAPHICS = 0x01,
> > + DP_CONTENT_TYPE_PHOTO = 0x02,
> > + DP_CONTENT_TYPE_VIDEO = 0x03,
> > + DP_CONTENT_TYPE_GAME = 0x04,
> > +};
> > +
> > +/* DRM DP VSC SDP as per DP 1.4 spec */ struct drm_dp_vsc_sdp {
> > + unsigned char sdp_type; /* Secondary-data Packet Type */
> > + unsigned char revision; /* Revision Number */
>
> These comments seems self-explanatory, you can probably drop them.
>
Thank you for review in detail, I'll update everything that you commented.
> > + unsigned char length; /* Number of Valid Data Bytes */
> > + enum dp_colorspace colorspace;
> > + enum dp_colorimetry colorimetry;
> > + int bpc; /* bit per color */
> > + enum dp_dynamic_range dynamic_range;
> > + enum dp_content_type content_type;
> > +};
> > +
> > int drm_dp_psr_setup_time(const u8
> > psr_cap[EDP_PSR_RECEIVER_CAP_SIZE]);
> >
> > static inline int
> > --
> > 2.24.1
> >
> > ___
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [Intel-gfx] [PATCH 08/18] drm/i915/dp: Add logging function for DP VSC SDP
On Sat, 2020-02-01 at 14:43 +0200, Jani Nikula wrote:
> On Fri, 31 Jan 2020, Gwan-gyeong Mun
> wrote:
> > When receiving video it is very useful to be able to log DP VSC
> > SDP.
> > This greatly simplifies debugging.
>
> Seems like a lot of the functions should really be in drm core.
>
> BR,
> Jani.
Hi,
Okay, I'll move these logging functions to drm core.
Br,
G.G.
>
> > Signed-off-by: Gwan-gyeong Mun
> > ---
> > drivers/gpu/drm/i915/display/intel_dp.c | 173
> >
> > drivers/gpu/drm/i915/display/intel_dp.h | 4 +
> > 2 files changed, 177 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
> > b/drivers/gpu/drm/i915/display/intel_dp.c
> > index 6756030692c8..e33488222ac5 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > @@ -5090,6 +5090,179 @@ void intel_read_dp_sdp(struct intel_encoder
> > *encoder,
> > }
> > }
> >
> > +static const char *dp_colorspace_get_name(enum dp_colorspace
> > colorspace)
> > +{
> > + if (colorspace < 0 || colorspace > DP_COLORSPACE_RESERVED)
> > + return "Invalid";
> > +
> > + switch (colorspace) {
> > + case DP_COLORSPACE_RGB:
> > + return "RGB";
> > + case DP_COLORSPACE_YUV444:
> > + return "YUV444";
> > + case DP_COLORSPACE_YUV422:
> > + return "YUV422";
> > + case DP_COLORSPACE_YUV420:
> > + return "YUV420";
> > + case DP_COLORSPACE_Y_ONLY:
> > + return "Y_ONLY";
> > + case DP_COLORSPACE_RAW:
> > + return "RAW";
> > + default:
> > + return "Reserved";
> > + }
> > +}
> > +
> > +static const char *dp_colorimetry_get_name(enum dp_colorspace
> > colorspace,
> > + enum dp_colorimetry
> > colorimetry)
> > +{
> > + if (colorspace < 0 || colorspace > DP_COLORSPACE_RESERVED)
> > + return "Invalid";
> > +
> > + switch (colorimetry) {
> > + case DP_COLORIMETRY_DEFAULT:
> > + switch (colorspace) {
> > + case DP_COLORSPACE_RGB:
> > + return "sRGB";
> > + case DP_COLORSPACE_YUV444:
> > + case DP_COLORSPACE_YUV422:
> > + case DP_COLORSPACE_YUV420:
> > + return "BT.601";
> > + case DP_COLORSPACE_Y_ONLY:
> > + return "DICOM PS3.14";
> > + case DP_COLORSPACE_RAW:
> > + return "Custom Color Profile";
> > + default:
> > + return "Reserved";
> > + }
> > + case DP_COLORIMETRY_RGB_WIDE_FIXED: /* and
> > DP_COLORIMETRY_BT709_YCC */
> > + switch (colorspace) {
> > + case DP_COLORSPACE_RGB:
> > + return "Wide Fixed";
> > + case DP_COLORSPACE_YUV444:
> > + case DP_COLORSPACE_YUV422:
> > + case DP_COLORSPACE_YUV420:
> > + return "BT.709";
> > + default:
> > + return "Reserved";
> > + }
> > + case DP_COLORIMETRY_RGB_WIDE_FLOAT: /* and
> > DP_COLORIMETRY_XVYCC_601 */
> > + switch (colorspace) {
> > + case DP_COLORSPACE_RGB:
> > + return "Wide Float";
> > + case DP_COLORSPACE_YUV444:
> > + case DP_COLORSPACE_YUV422:
> > + case DP_COLORSPACE_YUV420:
> > + return "xvYCC 601";
> > + default:
> > + return "Reserved";
> > + }
> > + case DP_COLORIMETRY_OPRGB: /* and DP_COLORIMETRY_XVYCC_709 */
> > + switch (colorspace) {
> > + case DP_COLORSPACE_RGB:
> > + return "OpRGB";
> > + case DP_COLORSPACE_YUV444:
> > + case DP_COLORSPACE_YUV422:
> > + case DP_COLORSPACE_YUV420:
> > + return "xvYCC 709";
> > + default:
> > + return "Reserved";
> > + }
> > + case DP_COLORIMETRY_DCI_P3_RGB: /* and DP_COLORIMETRY_SYCC_601
> > */
> > + switch (colorspace) {
> > + case DP_COLORSPACE_RGB:
> > + return "DCI-P3";
> > + case DP_COLORSPACE_YUV444:
> > + case DP_COLORSPACE_YUV422:
> > + case DP_COLORSPACE_YUV420:
> > + return "sYCC 601";
> > + default:
> > + return "Reserved";
> > + }
> > + case DP_COLORIMETRY_RGB_CUSTOM: /* and DP_COLORIMETRY_OPYCC_601
> > */
> > + switch (colorspace) {
> > + case DP_COLORSPACE_RGB:
> > + return "Custom Profile";
> > + case DP_COLORSPACE_YUV444:
> > + case DP_COLORSPACE_YUV422:
> > + case DP_COLORSPACE_YUV420:
> > + return "OpYCC 601";
> > + default:
> > + return "Reserved";
> > + }
> > + case DP_COLORIMETRY_BT2020_RGB: /* and
> > DP_COLORIMETRY_BT2020_CYCC */
> > + switch (colorspace) {
> > + case
Re: [Intel-gfx] [PATCH 10/12] drm/i915: Document ILK+ pipe csc matrix better
On Thu, 2019-07-18 at 17:50 +0300, Ville Syrjala wrote:
> From: Ville Syrjälä
>
> Add comments to explain the ilk pipe csc operation a bit better.
>
> Signed-off-by: Ville Syrjälä
> ---
> drivers/gpu/drm/i915/display/intel_color.c | 26 +---
> --
> 1 file changed, 21 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_color.c
> b/drivers/gpu/drm/i915/display/intel_color.c
> index 23a84dd7989f..736c42720daf 100644
> --- a/drivers/gpu/drm/i915/display/intel_color.c
> +++ b/drivers/gpu/drm/i915/display/intel_color.c
> @@ -42,6 +42,21 @@
>
> #define LEGACY_LUT_LENGTH256
>
> +/*
> + * ILK+ csc matrix:
> + *
> + * |R/Cr| | c0 c1 c2 | ( |R/Cr| |preoff0| ) |postoff0|
> + * |G/Y | = | c3 c4 c5 | x ( |G/Y | + |preoff1| ) + |postoff1|
> + * |B/Cb| | c6 c7 c8 | ( |B/Cb| |preoff2| ) |postoff2|
> + *
> + * ILK/SNB don't have explicit post offsets, and instead
> + * CSC_MODE_YUV_TO_RGB and CSC_BLACK_SCREEN_OFFSET are used:
> + * CSC_MODE_YUV_TO_RGB=0 + CSC_BLACK_SCREEN_OFFSET=0 -> 1/2, 0, 1/2
> + * CSC_MODE_YUV_TO_RGB=0 + CSC_BLACK_SCREEN_OFFSET=1 -> 1/2, 1/16,
> 1/2
It seems that the calculated values are assumes ITU-R BT.709 spec,
if you don't mind, can we add some comments which is based on ITU-R
BT.709?
> + * CSC_MODE_YUV_TO_RGB=1 + CSC_BLACK_SCREEN_OFFSET=0 -> 0, 0, 0
> + * CSC_MODE_YUV_TO_RGB=1 + CSC_BLACK_SCREEN_OFFSET=1 -> 1/16, 1/16,
> 1/16
> + */
> +
> /*
> * Extract the CSC coefficient from a CTM coefficient (in U32.32
> fixed point
> * format). This macro takes the coefficient we want transformed and
> the
> @@ -59,37 +74,38 @@
>
> #define ILK_CSC_POSTOFF_LIMITED_RANGE (16 * (1 << 12) / 255)
>
> +/* Nop pre/post offsets */
> static const u16 ilk_csc_off_zero[3] = {};
>
> +/* Identity matrix */
> static const u16 ilk_csc_coeff_identity[9] = {
> ILK_CSC_COEFF_1_0, 0, 0,
> 0, ILK_CSC_COEFF_1_0, 0,
> 0, 0, ILK_CSC_COEFF_1_0,
> };
>
> +/* Limited range RGB post offsets */
> static const u16 ilk_csc_postoff_limited_range[3] = {
> ILK_CSC_POSTOFF_LIMITED_RANGE,
> ILK_CSC_POSTOFF_LIMITED_RANGE,
> ILK_CSC_POSTOFF_LIMITED_RANGE,
> };
>
> +/* Full range RGB -> limited range RGB matrix */
> static const u16 ilk_csc_coeff_limited_range[9] = {
> ILK_CSC_COEFF_LIMITED_RANGE, 0, 0,
> 0, ILK_CSC_COEFF_LIMITED_RANGE, 0,
> 0, 0, ILK_CSC_COEFF_LIMITED_RANGE,
> };
>
> -/*
> - * These values are direct register values specified in the Bspec,
> - * for RGB->YUV conversion matrix (colorspace BT709)
> - */
> +/* BT.709 full range RGB -> limited range YCbCr matrix */
> static const u16 ilk_csc_coeff_rgb_to_ycbcr[9] = {
> 0x1e08, 0x9cc0, 0xb528,
> 0x2ba8, 0x09d8, 0x37e8,
> 0xbce8, 0x9ad8, 0x1e08,
> };
>
> -/* Post offset values for RGB->YCBCR conversion */
> +/* Limited range YCbCr post offsets */
> static const u16 ilk_csc_postoff_rgb_to_ycbcr[3] = {
> 0x0800, 0x0100, 0x0800,
> };
The changes look good to me.
Reviewed-by: Gwan-gyeong Mun
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [Intel-gfx] [PATCH 03/12] drm/i915: Fix AVI infoframe quantization range for YCbCr output
On Thu, 2019-07-18 at 17:50 +0300, Ville Syrjala wrote:
> From: Ville Syrjälä
>
> We're configuring the AVI infoframe quantization range bits as if
> we're always transmitting RGB pixels. Let's fix this so that we
> correctly indicate limited range YCC quantization range when
> transmitting YCbCr instead.
>
> Signed-off-by: Ville Syrjälä
> ---
> drivers/gpu/drm/i915/display/intel_hdmi.c | 15 ++-
> 1 file changed, 10 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c
> b/drivers/gpu/drm/i915/display/intel_hdmi.c
> index 9bf28de10401..b8100cf21dd0 100644
> --- a/drivers/gpu/drm/i915/display/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/display/intel_hdmi.c
> @@ -724,11 +724,16 @@ intel_hdmi_compute_avi_infoframe(struct
> intel_encoder *encoder,
>
> drm_hdmi_avi_infoframe_colorspace(frame, conn_state);
>
> - drm_hdmi_avi_infoframe_quant_range(frame, connector,
> -adjusted_mode,
> -crtc_state-
> >limited_color_range ?
> -HDMI_QUANTIZATION_RANGE_LIMI
> TED :
> -HDMI_QUANTIZATION_RANGE_FULL
> );
> + if (crtc_state->output_format == INTEL_OUTPUT_FORMAT_RGB) {
> + drm_hdmi_avi_infoframe_quant_range(frame, connector,
> +adjusted_mode,
> +crtc_state-
> >limited_color_range ?
> +HDMI_QUANTIZATION_RA
> NGE_LIMITED :
> +HDMI_QUANTIZATION_RA
> NGE_FULL);
> + } else {
> + frame->quantization_range =
> HDMI_QUANTIZATION_RANGE_DEFAULT;
> + frame->ycc_quantization_range =
> HDMI_YCC_QUANTIZATION_RANGE_LIMITED;
> + }
>
> drm_hdmi_avi_infoframe_content_type(frame, conn_state);
>
The changes look good to me.
Reviewed-by: Gwan-gyeong Mun
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [Intel-gfx] [PATCH 12/12] drm/i915: Add PIPECONF YCbCr 4:4:4 programming for ILK-IVB
Except typo, the changes look good to me.
Reviewed-by: Gwan-gyeong Mun
On Wed, 2019-09-18 at 19:05 +, Mun, Gwan-gyeong wrote:
> On Thu, 2019-07-18 at 17:50 +0300, Ville Syrjala wrote:
> > From: Ville Syrjälä
> >
> > On ILK-IVB the pipe colorspace is configured via PIPECONF
> > (as opposed to PIPEMISC in BDW+). Let's configure+readout
> > that stuff correctly.
> >
> > Enablling YCbCr 4:4:4 output will now be a simple matter of
> Typo: Enablling -> Enabling
> > setting crtc_state->output_format appropriately in the encoder
> > .compute_config(). However, when we do that we must be
> > aware of the fact that YCbCr DP output doesn't seem to work
> > on ILK (resulting image is totally garbled), but on SNB+
> > it works fine. However HDMI YCbCr output does work correctly
> > even on ILK.
> >
> > Signed-off-by: Ville Syrjälä
> > ---
> > drivers/gpu/drm/i915/display/intel_display.c | 21
> > +++-
> > drivers/gpu/drm/i915/i915_reg.h | 4
> > 2 files changed, 24 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> > b/drivers/gpu/drm/i915/display/intel_display.c
> > index bd3ff96c1618..8e98715cd63b 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -9406,9 +9406,19 @@ static void ironlake_set_pipeconf(const
> > struct
> > intel_crtc_state *crtc_state)
> > else
> > val |= PIPECONF_PROGRESSIVE;
> >
> > + /*
> > +* This would end up with an odd purple hue over
> > +* the entire display. Make sure we don't do it.
> > +*/
> > + WARN_ON(crtc_state->limited_color_range &&
> > + crtc_state->output_format != INTEL_OUTPUT_FORMAT_RGB);
> > +
> > if (crtc_state->limited_color_range)
> > val |= PIPECONF_COLOR_RANGE_SELECT;
> >
> > + if (crtc_state->output_format != INTEL_OUTPUT_FORMAT_RGB)
> > + val |= PIPECONF_OUTPUT_COLORSPACE_YUV709;
> > +
> > val |= PIPECONF_GAMMA_MODE(crtc_state->gamma_mode);
> >
> > I915_WRITE(PIPECONF(pipe), val);
> > @@ -9945,7 +9955,6 @@ static bool ironlake_get_pipe_config(struct
> > intel_crtc *crtc,
> > if (!wakeref)
> > return false;
> >
> > - pipe_config->output_format = INTEL_OUTPUT_FORMAT_RGB;
> > pipe_config->cpu_transcoder = (enum transcoder) crtc->pipe;
> > pipe_config->shared_dpll = NULL;
> >
> > @@ -9974,6 +9983,16 @@ static bool ironlake_get_pipe_config(struct
> > intel_crtc *crtc,
> > if (tmp & PIPECONF_COLOR_RANGE_SELECT)
> > pipe_config->limited_color_range = true;
> >
> > + switch (tmp & PIPECONF_OUTPUT_COLORSPACE_MASK) {
> > + case PIPECONF_OUTPUT_COLORSPACE_YUV601:
> > + case PIPECONF_OUTPUT_COLORSPACE_YUV709:
> > + pipe_config->output_format =
> > INTEL_OUTPUT_FORMAT_YCBCR444;
> > + break;
> > + default:
> > + pipe_config->output_format = INTEL_OUTPUT_FORMAT_RGB;
> > + break;
> > + }
> > +
> > pipe_config->gamma_mode = (tmp & PIPECONF_GAMMA_MODE_MASK_ILK)
> > PIPECONF_GAMMA_MODE_SHIFT;
> >
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > b/drivers/gpu/drm/i915/i915_reg.h
> > index 33d535ae0944..3d33a1e03a45 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -5712,6 +5712,10 @@ enum {
> > #define PIPECONF_CXSR_DOWNCLOCK (1 << 16)
> > #define PIPECONF_EDP_RR_MODE_SWITCH_VLV (1 << 14)
> > #define PIPECONF_COLOR_RANGE_SELECT (1 << 13)
> > +#define PIPECONF_OUTPUT_COLORSPACE_MASK (3 << 11) /* ilk-ivb */
> > +#define PIPECONF_OUTPUT_COLORSPACE_RGB (0 << 11) /* ilk-ivb */
> > +#define PIPECONF_OUTPUT_COLORSPACE_YUV601(1 << 11) /*
> > ilk-ivb */
> > +#define PIPECONF_OUTPUT_COLORSPACE_YUV709(2 << 11) /*
> > ilk-ivb */
> > #define PIPECONF_OUTPUT_COLORSPACE_YUV_HSW (1 << 11) /*
> > hsw only
> > */
> > #define PIPECONF_BPC_MASK(0x7 << 5)
> > #define PIPECONF_8BPC(0 << 5)
> ___
> Intel-gfx mailing list
> intel-...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [Intel-gfx] [PATCH 09/12] drm/i915: Add PIPECONF YCbCr 4:4:4 programming for HSW
Except typo, the changes look good to me.
Reviewed-by: Gwan-gyeong Mun
On Wed, 2019-09-18 at 19:03 +, Mun, Gwan-gyeong wrote:
> On Thu, 2019-07-18 at 17:50 +0300, Ville Syrjala wrote:
> > From: Ville Syrjälä
> >
> > On HSW the pipe colorspace is configured via PIPECONF
> > (as opposed to PIPEMISC in BDW+). Let's configure+readout
> > that stuff correctly.
> >
> > Enablling YCbCr 4:4:4 output will now be a simple matter of
> Typo: Enablling -> Enabling
> > setting crtc_state->output_format appropriately in the encoder
> > .compute_config().
> >
> > Signed-off-by: Ville Syrjälä
> > ---
> > drivers/gpu/drm/i915/display/intel_display.c | 13 -
> > drivers/gpu/drm/i915/i915_reg.h | 1 +
> > 2 files changed, 13 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> > b/drivers/gpu/drm/i915/display/intel_display.c
> > index 1dd1aa29a649..bd3ff96c1618 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -9430,6 +9430,10 @@ static void haswell_set_pipeconf(const
> > struct
> > intel_crtc_state *crtc_state)
> > else
> > val |= PIPECONF_PROGRESSIVE;
> >
> > + if (IS_HASWELL(dev_priv) &&
> > + crtc_state->output_format != INTEL_OUTPUT_FORMAT_RGB)
> > + val |= PIPECONF_OUTPUT_COLORSPACE_YUV_HSW;
> > +
> > I915_WRITE(PIPECONF(cpu_transcoder), val);
> > POSTING_READ(PIPECONF(cpu_transcoder));
> > }
> > @@ -10423,7 +10427,14 @@ static bool haswell_get_pipe_config(struct
> > intel_crtc *crtc,
> >
> > intel_get_pipe_src_size(crtc, pipe_config);
> >
> > - if (INTEL_GEN(dev_priv) >= 9 || IS_BROADWELL(dev_priv)) {
> > + if (IS_HASWELL(dev_priv)) {
> > + u32 tmp = I915_READ(PIPECONF(pipe_config-
> > > cpu_transcoder));
> > +
> > + if (tmp & PIPECONF_OUTPUT_COLORSPACE_YUV_HSW)
> > + pipe_config->output_format =
> > INTEL_OUTPUT_FORMAT_YCBCR444;
> > + else
> > + pipe_config->output_format =
> > INTEL_OUTPUT_FORMAT_RGB;
> > + } else {
> > pipe_config->output_format =
> > bdw_get_pipemisc_output_format(crtc);
> >
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > b/drivers/gpu/drm/i915/i915_reg.h
> > index 66f7f417231f..58471312b8b2 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -5712,6 +5712,7 @@ enum {
> > #define PIPECONF_CXSR_DOWNCLOCK (1 << 16)
> > #define PIPECONF_EDP_RR_MODE_SWITCH_VLV (1 << 14)
> > #define PIPECONF_COLOR_RANGE_SELECT (1 << 13)
> > +#define PIPECONF_OUTPUT_COLORSPACE_YUV_HSW (1 << 11) /*
> > hsw only
> > */
> > #define PIPECONF_BPC_MASK(0x7 << 5)
> > #define PIPECONF_8BPC(0 << 5)
> > #define PIPECONF_10BPC (1 << 5)
> ___
> Intel-gfx mailing list
> intel-...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [Intel-gfx] [PATCH 11/12] drm/i915: Set up ILK/SNB csc unit properly for YCbCr output
On Thu, 2019-07-18 at 17:50 +0300, Ville Syrjala wrote:
> From: Ville Syrjälä
>
> Prepare the pipe csc for YCbCr output on ilk/snb. The main difference
> to IVB+ is the lack of explicit post offsets, and instead we must
> configure the CSC info RGB->YUV mode (which takes care of offsetting
> Cb/Cr properly) and enable the "black screen offset" bit to add the
> required offset to Y.
>
> And while at it throw some comments around the bit defines to
> document which platforms have which bits.
>
> Signed-off-by: Ville Syrjälä
> ---
> drivers/gpu/drm/i915/display/intel_color.c | 25 +---
> --
> drivers/gpu/drm/i915/i915_reg.h| 10 -
> 2 files changed, 25 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_color.c
> b/drivers/gpu/drm/i915/display/intel_color.c
> index 736c42720daf..a902f7809840 100644
> --- a/drivers/gpu/drm/i915/display/intel_color.c
> +++ b/drivers/gpu/drm/i915/display/intel_color.c
> @@ -1213,6 +1213,21 @@ static u32 ilk_gamma_mode(const struct
> intel_crtc_state *crtc_state)
> return GAMMA_MODE_MODE_10BIT;
> }
>
> +static u32 ilk_csc_mode(const struct intel_crtc_state *crtc_state)
> +{
> + /*
> + * CSC comes after the LUT in RGB->YCbCr mode.
> + * RGB->YCbCr needs the limited range offsets added to
> + * the output. RGB limited range output is handled by
> + * the hw automagically elsewhere.
> + */
> + if (crtc_state->output_format != INTEL_OUTPUT_FORMAT_RGB)
> + return CSC_BLACK_SCREEN_OFFSET;
> +
> + return CSC_MODE_YUV_TO_RGB |
> + CSC_POSITION_BEFORE_GAMMA;
> +}
> +
> static int ilk_color_check(struct intel_crtc_state *crtc_state)
> {
> int ret;
> @@ -1226,15 +1241,15 @@ static int ilk_color_check(struct
> intel_crtc_state *crtc_state)
> !crtc_state->c8_planes;
>
> /*
> - * We don't expose the ctm on ilk/snb currently,
> - * nor do we enable YCbCr output. Also RGB limited
> - * range output is handled by the hw automagically.
> + * We don't expose the ctm on ilk/snb currently, also RGB
> + * limited range output is handled by the hw automagically.
>*/
> - crtc_state->csc_enable = false;
> + crtc_state->csc_enable =
> + crtc_state->output_format != INTEL_OUTPUT_FORMAT_RGB;
>
> crtc_state->gamma_mode = ilk_gamma_mode(crtc_state);
>
> - crtc_state->csc_mode = 0;
> + crtc_state->csc_mode = ilk_csc_mode(crtc_state);
>
> ret = intel_color_add_affected_planes(crtc_state);
> if (ret)
> diff --git a/drivers/gpu/drm/i915/i915_reg.h
> b/drivers/gpu/drm/i915/i915_reg.h
> index 58471312b8b2..33d535ae0944 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -10106,11 +10106,11 @@ enum skl_power_gate {
> #define _PIPE_A_CSC_COEFF_BV 0x49024
>
> #define _PIPE_A_CSC_MODE 0x49028
> -#define ICL_CSC_ENABLE (1 << 31)
> -#define ICL_OUTPUT_CSC_ENABLE (1 << 30)
> -#define CSC_BLACK_SCREEN_OFFSET (1 << 2)
> -#define CSC_POSITION_BEFORE_GAMMA (1 << 1)
> -#define CSC_MODE_YUV_TO_RGB (1 << 0)
> +#define ICL_CSC_ENABLE (1 << 31) /* icl+ */
> +#define ICL_OUTPUT_CSC_ENABLE (1 << 30) /* icl+ */
> +#define CSC_BLACK_SCREEN_OFFSET (1 << 2) /* ilk/snb */
> +#define CSC_POSITION_BEFORE_GAMMA (1 << 1) /* pre-glk */
> +#define CSC_MODE_YUV_TO_RGB (1 << 0) /* ilk/snb */
>
> #define _PIPE_A_CSC_PREOFF_HI0x49030
> #define _PIPE_A_CSC_PREOFF_ME0x49034
The changes look good to me.
Reviewed-by: Gwan-gyeong Mun
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v8 6/7] drm/i915/dp: Program an Infoframe SDP Header and DB for HDR Static Metadata
On Wed, 2019-09-18 at 17:13 +0300, Ville Syrjälä wrote:
> On Mon, Sep 16, 2019 at 10:11:49AM +0300, Gwan-gyeong Mun wrote:
> > Function intel_dp_setup_hdr_metadata_infoframe_sdp handles
> > Infoframe SDP
> > header and data block setup for HDR Static Metadata. It enables
> > writing of
> > HDR metadata infoframe SDP to panel. Support for HDR video was
> > introduced
> > in DisplayPort 1.4. It implements the CTA-861-G standard for
> > transport of
> > static HDR metadata. The HDR Metadata will be provided by userspace
> > compositors, based on blending policies and passed to the driver
> > through
> > a blob property.
> >
> > Because each of GEN11 and prior GEN11 have different register size
> > for
> > HDR Metadata Infoframe SDP packet, it adds and uses different
> > register
> > size.
> >
> > Setup Infoframe SDP header and data block in function
> > intel_dp_setup_hdr_metadata_infoframe_sdp for HDR Static Metadata
> > as per
> > dp 1.4 spec and CTA-861-F spec.
> > As per DP 1.4 spec, 2.2.2.5 SDP Formats. It enables Dynamic Range
> > and
> > Mastering Infoframe for HDR content, which is defined in CTA-861-F
> > spec.
> > According to DP 1.4 spec and CEA-861-F spec Table 5, in order to
> > transmit
> > static HDR metadata, we have to use Non-audio INFOFRAME SDP v1.3.
> >
> > ++---+
> > > [ Packet Type Value ] | [ Packet Type ] |
> > ++---+
> > > 80h + Non-audio INFOFRAME Type | CEA-861-F Non-audio INFOFRAME |
> > ++---+
> > > [Transmission Timing] |
> > ++
> > > As per CEA-861-F for INFOFRAME, including CEA-861.3 within |
> > > which Dynamic Range and Mastering INFOFRAME are defined|
> > ++
> >
> > v2: Add a missed blank line after function declaration.
> > v3: Remove not handled return values from
> > intel_dp_setup_hdr_metadata_infoframe_sdp(). [Uma]
> >
> > Signed-off-by: Gwan-gyeong Mun
> > Reviewed-by: Uma Shankar
> > ---
> > drivers/gpu/drm/i915/display/intel_ddi.c | 1 +
> > drivers/gpu/drm/i915/display/intel_dp.c | 89
> >
> > drivers/gpu/drm/i915/display/intel_dp.h | 3 +
> > 3 files changed, 93 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c
> > b/drivers/gpu/drm/i915/display/intel_ddi.c
> > index 8dc030650801..306f6f9f0204 100644
> > --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> > @@ -3625,6 +3625,7 @@ static void intel_enable_ddi_dp(struct
> > intel_encoder *encoder,
> > intel_edp_backlight_on(crtc_state, conn_state);
> > intel_psr_enable(intel_dp, crtc_state);
> > intel_dp_vsc_enable(intel_dp, crtc_state, conn_state);
> > + intel_dp_hdr_metadata_enable(intel_dp, crtc_state, conn_state);
> > intel_edp_drrs_enable(intel_dp, crtc_state);
> >
> > if (crtc_state->has_audio)
> > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
> > b/drivers/gpu/drm/i915/display/intel_dp.c
> > index 7fe22b37474d..abbf1d5c54c4 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > @@ -4599,6 +4599,83 @@ intel_dp_setup_vsc_sdp(struct intel_dp
> > *intel_dp,
> > crtc_state, DP_SDP_VSC, _sdp,
> > sizeof(vsc_sdp));
> > }
> >
> > +static void
> > +intel_dp_setup_hdr_metadata_infoframe_sdp(struct intel_dp
> > *intel_dp,
> > + const struct intel_crtc_state
> > *crtc_state,
> > + const struct
> > drm_connector_state *conn_state)
> > +{
> > + struct intel_digital_port *intel_dig_port =
> > dp_to_dig_port(intel_dp);
> > + struct drm_i915_private *dev_priv = to_i915(intel_dig_port-
> > >base.base.dev);
> > + struct dp_sdp infoframe_sdp = {};
> > + struct hdmi_drm_infoframe drm_infoframe = {};
> > + const int infoframe_size = HDMI_INFOFRAME_HEADER_SIZE +
> > HDMI_DRM_INFOFRAME_SIZE;
> > + unsigned char buf[HDMI_INFOFRAME_HEADER_SIZE +
> > HDMI_DRM_INFOFRAME_SIZE];
> > + ssize_t len;
> > + int ret;
> > +
> > + ret = drm_hdmi_infoframe_set_hdr_metadata(_infoframe,
> > conn_state);
> > + if (ret) {
> > + DRM_DEBUG_KMS("couldn't set HDR metadata in
> > infoframe\n");
> > + return;
> > + }
> > +
> > + len = hdmi_drm_infoframe_pack_only(_infoframe, buf,
> > sizeof(buf));
> > + if (len < 0) {
> > + DRM_DEBUG_KMS("buffer size is smaller than hdr metadata
> > infoframe\n");
> > + return;
> > + }
> > +
> > + if (len != infoframe_size) {
> > + DRM_DEBUG_KMS("wrong static hdr metadata size\n");
> > + return;
> > + }
> > +
> > + /*
> > +* Set up the infoframe sdp packet
Re: [PATCH v8 3/7] drm: Add DisplayPort colorspace property
On Wed, 2019-09-18 at 17:08 +0300, Ville Syrjälä wrote:
> On Mon, Sep 16, 2019 at 10:11:46AM +0300, Gwan-gyeong Mun wrote:
> > Because between HDMI and DP have different colorspaces, it renames
> > drm_mode_create_colorspace_property() function to
> > drm_mode_create_hdmi_colorspace_property() function for HDMI
> > connector.
> > And it adds drm_mode_create_dp_colorspace_property() function for
> > creating
> > of DP colorspace property.
> > In order to apply changed and added drm api, i915 driver has
> > channged.
> >
> > v3: Addressed review comments from Ville
> > - Add new colorimetry options for DP 1.4a spec.
> > - Separate set of colorimetry enum values for DP.
> > v4: Add additional comments to struct drm_prop_enum_list.
> > Polishing an enum string of struct drm_prop_enum_list
> > v5: Change definitions of DRM_MODE_COLORIMETRYs to follow HDMI
> > prefix and
> > DP abbreviations.
> > Add missed variables on dp_colorspaces.
> > Fix typo. [Uma]
> > v6: Addressed review comments from Ilia and Ville
> >- Split drm_mode_create_colorspace_property() to DP and HDMI
> > connector.
> > v7: Fix typo [Jani Saarinen]
> > Fix white space.
> > v8: Addressed review comments from Ville
> >- Drop colorimetries which have another way to distinguish or
> > which
> > would not be used.
> >
> > Signed-off-by: Gwan-gyeong Mun
> > Reviewed-by: Uma Shankar
> > ---
> > drivers/gpu/drm/drm_connector.c | 101
> > +++---
> > .../gpu/drm/i915/display/intel_connector.c| 21 +++-
>
> The i915 part shouldn't be here. Looks like you can just move that
> hunk into the next patch.
>
Okay, I'll split hunk into renaming and adding of code.
> > include/drm/drm_connector.h | 7 +-
> > 3 files changed, 108 insertions(+), 21 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_connector.c
> > b/drivers/gpu/drm/drm_connector.c
> > index 4c766624b20d..57c97949081a 100644
> > --- a/drivers/gpu/drm/drm_connector.c
> > +++ b/drivers/gpu/drm/drm_connector.c
> > @@ -882,6 +882,38 @@ static const struct drm_prop_enum_list
> > hdmi_colorspaces[] = {
> > { DRM_MODE_COLORIMETRY_DCI_P3_RGB_THEATER, "DCI-P3_RGB_Theater"
> > },
> > };
> >
> > +/*
> > + * As per DP 1.4a spec, 2.2.5.7.5 VSC SDP Payload for Pixel
> > Encoding/Colorimetry
> > + * Format Table 2-120
> > + */
> > +static const struct drm_prop_enum_list dp_colorspaces[] = {
> > + /* For Default case, driver will set the colorspace */
> > + { DRM_MODE_COLORIMETRY_DEFAULT, "Default" },
> > + { DRM_MODE_COLORIMETRY_RGB_WIDE_FIXED,
> > "RGB_Wide_Gamut_Fixed_Point" },
> > + /* Colorimetry based on scRGB (IEC 61966-2-2) */
> > + { DRM_MODE_COLORIMETRY_RGB_WIDE_FLOAT,
> > "RGB_Wide_Gamut_Floating_Point" },
> > + /* Colorimetry based on IEC 61966-2-5 */
> > + { DRM_MODE_COLORIMETRY_OPRGB, "opRGB" },
> > + /* Colorimetry based on SMPTE RP 431-2 */
> > + { DRM_MODE_COLORIMETRY_DCI_P3_RGB_D65, "DCI-P3_RGB_D65" },
> > + /* Colorimetry based on ITU-R BT.2020 */
> > + { DRM_MODE_COLORIMETRY_BT2020_RGB, "BT2020_RGB" },
> > + { DRM_MODE_COLORIMETRY_BT601_YCC, "BT601_YCC" },
> > + { DRM_MODE_COLORIMETRY_BT709_YCC, "BT709_YCC" },
> > + /* Standard Definition Colorimetry based on IEC 61966-2-4 */
> > + { DRM_MODE_COLORIMETRY_XVYCC_601, "XVYCC_601" },
> > + /* High Definition Colorimetry based on IEC 61966-2-4 */
> > + { DRM_MODE_COLORIMETRY_XVYCC_709, "XVYCC_709" },
> > + /* Colorimetry based on IEC 61966-2-1/Amendment 1 */
> > + { DRM_MODE_COLORIMETRY_SYCC_601, "SYCC_601" },
> > + /* Colorimetry based on IEC 61966-2-5 [33] */
> > + { DRM_MODE_COLORIMETRY_OPYCC_601, "opYCC_601" },
> > + /* Colorimetry based on ITU-R BT.2020 */
> > + { DRM_MODE_COLORIMETRY_BT2020_CYCC, "BT2020_CYCC" },
> > + /* Colorimetry based on ITU-R BT.2020 */
> > + { DRM_MODE_COLORIMETRY_BT2020_YCC, "BT2020_YCC" },
> > +};
> > +
> > /**
> > * DOC: standard connector properties
> > *
> > @@ -1674,7 +1706,6 @@
> > EXPORT_SYMBOL(drm_mode_create_aspect_ratio_property);
> > * DOC: standard connector properties
> > *
> > * Colorspace:
> > - * drm_mode_create_colorspace_property - create colorspace
> > property
> > * This property helps select a suitable colorspace based on
> > the sink
> > * capability. Modern sink devices support wider gamut like
> > BT2020.
> > * This helps switch to BT2020 mode if the BT2020 encoded
> > video stream
> > @@ -1694,32 +1725,68 @@
> > EXPORT_SYMBOL(drm_mode_create_aspect_ratio_property);
> > * - This property is just to inform sink what colorspace
> > *source is trying to drive.
> > *
> > + * Because between HDMI and DP have different colorspaces,
> > + * drm_mode_create_hdmi_colorspace_property() is used for HDMI
> > connector and
> > + * drm_mode_create_dp_colorspace_property() is used for DP
> > connector.
> > + */
> > +
> > +/**
> > + * drm_mode_create_hdmi_colorspace_property - create hdmi
> >
Re: [PATCH v8 2/7] drm/i915/dp: Add support of BT.2020 Colorimetry to DP MSA
On Wed, 2019-09-18 at 17:15 +0300, Ville Syrjälä wrote:
> On Mon, Sep 16, 2019 at 10:11:45AM +0300, Gwan-gyeong Mun wrote:
> > When BT.2020 Colorimetry output is used for DP, we should program
> > BT.2020
> > Colorimetry to MSA and VSC SDP. It adds output_colorspace to
> > intel_crtc_state struct as a place holder of pipe's output
> > colorspace.
> > In order to distinguish needed colorimetry for VSC SDP, it adds
> > intel_dp_needs_vsc_sdp function.
> > If the output colorspace requires vsc sdp or output format is YCbCr
> > 4:2:0,
> > it uses MSA with VSC SDP.
> >
> > As per DP 1.4a spec section 2.2.4.3 [MSA Field for Indication of
> > Color Encoding Format and Content Color Gamut] while sending
> > BT.2020 Colorimetry signals we should program MSA MISC1 fields
> > which
> > indicate VSC SDP for the Pixel Encoding/Colorimetry Format.
> >
> > v2: Remove useless parentheses
> > v3: Addressed review comments from Ville
> > - In order to checking output format and output colorspace on
> > intel_dp_needs_vsc_sdp(), it passes entire intel_crtc_state
> > struct
> > value.
> > - Remove a pointless variable.
> >
> > Signed-off-by: Gwan-gyeong Mun
> > Reviewed-by: Uma Shankar
> > ---
> > drivers/gpu/drm/i915/display/intel_ddi.c | 7 +++--
> > .../drm/i915/display/intel_display_types.h| 3 ++
> > drivers/gpu/drm/i915/display/intel_dp.c | 29
> > ++-
> > drivers/gpu/drm/i915/display/intel_dp.h | 1 +
> > 4 files changed, 36 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c
> > b/drivers/gpu/drm/i915/display/intel_ddi.c
> > index 98d69febd8e3..8dc030650801 100644
> > --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> > @@ -1737,11 +1737,12 @@ void intel_ddi_set_pipe_settings(const
> > struct intel_crtc_state *crtc_state)
> > /*
> > * As per DP 1.4a spec section 2.2.4.3 [MSA Field for
> > Indication
> > * of Color Encoding Format and Content Color Gamut] while
> > sending
> > -* YCBCR 420 signals we should program MSA MISC1 fields which
> > -* indicate VSC SDP for the Pixel Encoding/Colorimetry Format.
> > +* YCBCR 420, HDR BT.2020 signals we should program MSA MISC1
> > fields
> > +* which indicate VSC SDP for the Pixel Encoding/Colorimetry
> > Format.
> > */
> > - if (crtc_state->output_format == INTEL_OUTPUT_FORMAT_YCBCR420)
> > + if (intel_dp_needs_vsc_sdp(crtc_state))
> > temp |= TRANS_MSA_USE_VSC_SDP;
> > +
> > I915_WRITE(TRANS_MSA_MISC(cpu_transcoder), temp);
> > }
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h
> > b/drivers/gpu/drm/i915/display/intel_display_types.h
> > index d5cc4b810d9e..4108570907d4 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> > @@ -971,6 +971,9 @@ struct intel_crtc_state {
> > /* Output format RGB/YCBCR etc */
> > enum intel_output_format output_format;
> >
> > + /* Output colorspace sRGB/BT.2020 etc */
> > + u32 output_colorspace;
>
> Why are we duplicating this? It's already in the connector state no?
I'll remove a duplicated output color space from intel_crtc_state. And
in order to handle colorspace of drm_connector_state, I'll moves a
calling of intel_ddi_set_pipe_settings() function into
intel_ddi_pre_enable_dp().
>
> > +
> > /* Output down scaling is done in LSPCON device */
> > bool lspcon_downsampling;
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
> > b/drivers/gpu/drm/i915/display/intel_dp.c
> > index a2a0214f771a..3a8aef1c6036 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > @@ -2187,6 +2187,8 @@ intel_dp_compute_config(struct intel_encoder
> > *encoder,
> > pipe_config->has_pch_encoder = true;
> >
> > pipe_config->output_format = INTEL_OUTPUT_FORMAT_RGB;
> > + pipe_config->output_colorspace = intel_conn_state-
> > >base.colorspace;
> > +
> > if (lspcon->active)
> > lspcon_ycbcr420_config(_connector->base,
> > pipe_config);
> > else
> > @@ -4448,6 +4450,31 @@ u8 intel_dp_dsc_get_slice_count(struct
> > intel_dp *intel_dp,
> > return 0;
> > }
> >
> > +bool
> > +intel_dp_needs_vsc_sdp(const struct intel_crtc_state *crtc_state)
> > +{
> > + /*
> > +* As per DP 1.4a spec section 2.2.4.3 [MSA Field for
> > Indication
> > +* of Color Encoding Format and Content Color Gamut], in order
> > to
> > +* sending YCBCR 420 or HDR BT.2020 signals we should use DP
> > VSC SDP.
> > +*/
> > + if (crtc_state->output_format == INTEL_OUTPUT_FORMAT_YCBCR420)
> > + return true;
> > +
> > + switch (crtc_state->output_colorspace) {
> > + case DRM_MODE_COLORIMETRY_SYCC_601:
> > + case DRM_MODE_COLORIMETRY_OPYCC_601:
> > + case DRM_MODE_COLORIMETRY_BT2020_YCC:
> > + case
Re: [Intel-gfx] [PATCH 12/12] drm/i915: Add PIPECONF YCbCr 4:4:4 programming for ILK-IVB
On Thu, 2019-07-18 at 17:50 +0300, Ville Syrjala wrote:
> From: Ville Syrjälä
>
> On ILK-IVB the pipe colorspace is configured via PIPECONF
> (as opposed to PIPEMISC in BDW+). Let's configure+readout
> that stuff correctly.
>
> Enablling YCbCr 4:4:4 output will now be a simple matter of
Typo: Enablling -> Enabling
> setting crtc_state->output_format appropriately in the encoder
> .compute_config(). However, when we do that we must be
> aware of the fact that YCbCr DP output doesn't seem to work
> on ILK (resulting image is totally garbled), but on SNB+
> it works fine. However HDMI YCbCr output does work correctly
> even on ILK.
>
> Signed-off-by: Ville Syrjälä
> ---
> drivers/gpu/drm/i915/display/intel_display.c | 21
> +++-
> drivers/gpu/drm/i915/i915_reg.h | 4
> 2 files changed, 24 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> b/drivers/gpu/drm/i915/display/intel_display.c
> index bd3ff96c1618..8e98715cd63b 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -9406,9 +9406,19 @@ static void ironlake_set_pipeconf(const struct
> intel_crtc_state *crtc_state)
> else
> val |= PIPECONF_PROGRESSIVE;
>
> + /*
> + * This would end up with an odd purple hue over
> + * the entire display. Make sure we don't do it.
> + */
> + WARN_ON(crtc_state->limited_color_range &&
> + crtc_state->output_format != INTEL_OUTPUT_FORMAT_RGB);
> +
> if (crtc_state->limited_color_range)
> val |= PIPECONF_COLOR_RANGE_SELECT;
>
> + if (crtc_state->output_format != INTEL_OUTPUT_FORMAT_RGB)
> + val |= PIPECONF_OUTPUT_COLORSPACE_YUV709;
> +
> val |= PIPECONF_GAMMA_MODE(crtc_state->gamma_mode);
>
> I915_WRITE(PIPECONF(pipe), val);
> @@ -9945,7 +9955,6 @@ static bool ironlake_get_pipe_config(struct
> intel_crtc *crtc,
> if (!wakeref)
> return false;
>
> - pipe_config->output_format = INTEL_OUTPUT_FORMAT_RGB;
> pipe_config->cpu_transcoder = (enum transcoder) crtc->pipe;
> pipe_config->shared_dpll = NULL;
>
> @@ -9974,6 +9983,16 @@ static bool ironlake_get_pipe_config(struct
> intel_crtc *crtc,
> if (tmp & PIPECONF_COLOR_RANGE_SELECT)
> pipe_config->limited_color_range = true;
>
> + switch (tmp & PIPECONF_OUTPUT_COLORSPACE_MASK) {
> + case PIPECONF_OUTPUT_COLORSPACE_YUV601:
> + case PIPECONF_OUTPUT_COLORSPACE_YUV709:
> + pipe_config->output_format =
> INTEL_OUTPUT_FORMAT_YCBCR444;
> + break;
> + default:
> + pipe_config->output_format = INTEL_OUTPUT_FORMAT_RGB;
> + break;
> + }
> +
> pipe_config->gamma_mode = (tmp & PIPECONF_GAMMA_MODE_MASK_ILK)
> >>
> PIPECONF_GAMMA_MODE_SHIFT;
>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h
> b/drivers/gpu/drm/i915/i915_reg.h
> index 33d535ae0944..3d33a1e03a45 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -5712,6 +5712,10 @@ enum {
> #define PIPECONF_CXSR_DOWNCLOCK(1 << 16)
> #define PIPECONF_EDP_RR_MODE_SWITCH_VLV(1 << 14)
> #define PIPECONF_COLOR_RANGE_SELECT(1 << 13)
> +#define PIPECONF_OUTPUT_COLORSPACE_MASK(3 << 11) /* ilk-ivb */
> +#define PIPECONF_OUTPUT_COLORSPACE_RGB (0 << 11) /* ilk-ivb */
> +#define PIPECONF_OUTPUT_COLORSPACE_YUV601 (1 << 11) /* ilk-ivb */
> +#define PIPECONF_OUTPUT_COLORSPACE_YUV709 (2 << 11) /* ilk-ivb */
> #define PIPECONF_OUTPUT_COLORSPACE_YUV_HSW (1 << 11) /* hsw only
> */
> #define PIPECONF_BPC_MASK (0x7 << 5)
> #define PIPECONF_8BPC (0 << 5)
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [Intel-gfx] [PATCH 09/12] drm/i915: Add PIPECONF YCbCr 4:4:4 programming for HSW
On Thu, 2019-07-18 at 17:50 +0300, Ville Syrjala wrote:
> From: Ville Syrjälä
>
> On HSW the pipe colorspace is configured via PIPECONF
> (as opposed to PIPEMISC in BDW+). Let's configure+readout
> that stuff correctly.
>
> Enablling YCbCr 4:4:4 output will now be a simple matter of
Typo: Enablling -> Enabling
> setting crtc_state->output_format appropriately in the encoder
> .compute_config().
>
> Signed-off-by: Ville Syrjälä
> ---
> drivers/gpu/drm/i915/display/intel_display.c | 13 -
> drivers/gpu/drm/i915/i915_reg.h | 1 +
> 2 files changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> b/drivers/gpu/drm/i915/display/intel_display.c
> index 1dd1aa29a649..bd3ff96c1618 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -9430,6 +9430,10 @@ static void haswell_set_pipeconf(const struct
> intel_crtc_state *crtc_state)
> else
> val |= PIPECONF_PROGRESSIVE;
>
> + if (IS_HASWELL(dev_priv) &&
> + crtc_state->output_format != INTEL_OUTPUT_FORMAT_RGB)
> + val |= PIPECONF_OUTPUT_COLORSPACE_YUV_HSW;
> +
> I915_WRITE(PIPECONF(cpu_transcoder), val);
> POSTING_READ(PIPECONF(cpu_transcoder));
> }
> @@ -10423,7 +10427,14 @@ static bool haswell_get_pipe_config(struct
> intel_crtc *crtc,
>
> intel_get_pipe_src_size(crtc, pipe_config);
>
> - if (INTEL_GEN(dev_priv) >= 9 || IS_BROADWELL(dev_priv)) {
> + if (IS_HASWELL(dev_priv)) {
> + u32 tmp = I915_READ(PIPECONF(pipe_config-
> >cpu_transcoder));
> +
> + if (tmp & PIPECONF_OUTPUT_COLORSPACE_YUV_HSW)
> + pipe_config->output_format =
> INTEL_OUTPUT_FORMAT_YCBCR444;
> + else
> + pipe_config->output_format =
> INTEL_OUTPUT_FORMAT_RGB;
> + } else {
> pipe_config->output_format =
> bdw_get_pipemisc_output_format(crtc);
>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h
> b/drivers/gpu/drm/i915/i915_reg.h
> index 66f7f417231f..58471312b8b2 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -5712,6 +5712,7 @@ enum {
> #define PIPECONF_CXSR_DOWNCLOCK(1 << 16)
> #define PIPECONF_EDP_RR_MODE_SWITCH_VLV(1 << 14)
> #define PIPECONF_COLOR_RANGE_SELECT(1 << 13)
> +#define PIPECONF_OUTPUT_COLORSPACE_YUV_HSW (1 << 11) /* hsw only
> */
> #define PIPECONF_BPC_MASK (0x7 << 5)
> #define PIPECONF_8BPC (0 << 5)
> #define PIPECONF_10BPC (1 << 5)
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [Intel-gfx] [PATCH 08/12] drm/i915: Simplify intel_get_crtc_ycbcr_config()
On Thu, 2019-07-18 at 17:50 +0300, Ville Syrjala wrote:
> From: Ville Syrjälä
>
> Make intel_get_crtc_ycbcr_config() simpler and rename it
> to bdw_get_pipemisc_output_format() to better reflect what
> it does.
>
> Also toss in some comments to document that the 4:2:0 PIPECONF
> bits are glk+ only. They are mbz on earlier platforms so reading
> them unconditionally is safe however.
>
> Signed-off-by: Ville Syrjälä
> ---
> drivers/gpu/drm/i915/display/intel_display.c | 71 +-
> --
> drivers/gpu/drm/i915/i915_reg.h | 4 +-
> 2 files changed, 34 insertions(+), 41 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> b/drivers/gpu/drm/i915/display/intel_display.c
> index ffdc350dc24a..1dd1aa29a649 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -8713,47 +8713,24 @@ static void chv_crtc_clock_get(struct
> intel_crtc *crtc,
> pipe_config->port_clock = chv_calc_dpll_params(refclk, );
> }
>
> -static void intel_get_crtc_ycbcr_config(struct intel_crtc *crtc,
> - struct intel_crtc_state
> *pipe_config)
> +static enum intel_output_format
> +bdw_get_pipemisc_output_format(struct intel_crtc *crtc)
> {
> struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> - enum intel_output_format output = INTEL_OUTPUT_FORMAT_RGB;
> -
> - pipe_config->lspcon_downsampling = false;
> -
> - if (IS_BROADWELL(dev_priv) || INTEL_GEN(dev_priv) >= 9) {
> - u32 tmp = I915_READ(PIPEMISC(crtc->pipe));
> -
> - if (tmp & PIPEMISC_OUTPUT_COLORSPACE_YUV) {
> - bool ycbcr420_enabled = tmp &
> PIPEMISC_YUV420_ENABLE;
> - bool blend = tmp &
> PIPEMISC_YUV420_MODE_FULL_BLEND;
> -
> - if (ycbcr420_enabled) {
> - /* We support 4:2:0 in full blend mode
> only */
> - if (!blend)
> - output =
> INTEL_OUTPUT_FORMAT_INVALID;
> - else if (!(IS_GEMINILAKE(dev_priv) ||
> -INTEL_GEN(dev_priv) >= 10))
> - output =
> INTEL_OUTPUT_FORMAT_INVALID;
> - else
> - output =
> INTEL_OUTPUT_FORMAT_YCBCR420;
> - } else {
> - /*
> - * Currently there is no interface
> defined to
> - * check user preference between
> RGB/YCBCR444
> - * or YCBCR420. So the only possible
> case for
> - * YCBCR444 usage is driving YCBCR420
> output
> - * with LSPCON, when pipe is configured
> for
> - * YCBCR444 output and LSPCON takes
> care of
> - * downsampling it.
> - */
> - pipe_config->lspcon_downsampling =
> true;
> - output = INTEL_OUTPUT_FORMAT_YCBCR444;
> - }
> - }
> - }
> + u32 tmp;
> +
> + tmp = I915_READ(PIPEMISC(crtc->pipe));
>
> - pipe_config->output_format = output;
> + if (tmp & PIPEMISC_YUV420_ENABLE) {
> + /* We support 4:2:0 in full blend mode only */
> + WARN_ON((tmp & PIPEMISC_YUV420_MODE_FULL_BLEND) == 0);
> +
> + return INTEL_OUTPUT_FORMAT_YCBCR420;
> + } else if (tmp & PIPEMISC_OUTPUT_COLORSPACE_YUV) {
> + return INTEL_OUTPUT_FORMAT_YCBCR444;
> + } else {
> + return INTEL_OUTPUT_FORMAT_RGB;
> + }
> }
>
> static void i9xx_get_pipe_color_config(struct intel_crtc_state
> *crtc_state)
> @@ -10445,7 +10422,23 @@ static bool haswell_get_pipe_config(struct
> intel_crtc *crtc,
> }
>
> intel_get_pipe_src_size(crtc, pipe_config);
> - intel_get_crtc_ycbcr_config(crtc, pipe_config);
> +
> + if (INTEL_GEN(dev_priv) >= 9 || IS_BROADWELL(dev_priv)) {
> + pipe_config->output_format =
> + bdw_get_pipemisc_output_format(crtc);
> +
> + /*
> + * Currently there is no interface defined to
> + * check user preference between RGB/YCBCR444
> + * or YCBCR420. So the only possible case for
> + * YCBCR444 usage is driving YCBCR420 output
> + * with LSPCON, when pipe is configured for
> + * YCBCR444 output and LSPCON takes care of
> + * downsampling it.
> + */
> + pipe_config->lspcon_downsampling =
> + pipe_config->output_format ==
> INTEL_OUTPUT_FORMAT_YCBCR444;
> + }
>
> pipe_config->gamma_mode = I915_READ(GAMMA_MODE(crtc->pipe));
>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h
>
Re: [Intel-gfx] [PATCH 06/12] drm/i915: Switch to using DP_MSA_MISC_* defines
On Thu, 2019-07-18 at 17:50 +0300, Ville Syrjala wrote:
> From: Ville Syrjälä
>
> Now that we have standard defines for the MSA MISC bits lets use
> them on HSW+ where we program these directly into the TRANS_MSA_MISC
> register.
>
> Signed-off-by: Ville Syrjälä
> ---
> drivers/gpu/drm/i915/display/intel_ddi.c | 18 +-
> drivers/gpu/drm/i915/i915_reg.h | 13 +
> 2 files changed, 10 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c
> b/drivers/gpu/drm/i915/display/intel_ddi.c
> index 7dd54f573f35..0c0148c8c996 100644
> --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> @@ -1704,20 +1704,20 @@ void intel_ddi_set_pipe_settings(const struct
> intel_crtc_state *crtc_state)
>
> WARN_ON(transcoder_is_dsi(cpu_transcoder));
>
> - temp = TRANS_MSA_SYNC_CLK;
> + temp = DP_MSA_MISC_SYNC_CLOCK;
>
> switch (crtc_state->pipe_bpp) {
> case 18:
> - temp |= TRANS_MSA_6_BPC;
> + temp |= DP_MSA_MISC_6_BPC;
> break;
> case 24:
> - temp |= TRANS_MSA_8_BPC;
> + temp |= DP_MSA_MISC_8_BPC;
> break;
> case 30:
> - temp |= TRANS_MSA_10_BPC;
> + temp |= DP_MSA_MISC_10_BPC;
> break;
> case 36:
> - temp |= TRANS_MSA_12_BPC;
> + temp |= DP_MSA_MISC_12_BPC;
> break;
> default:
> MISSING_CASE(crtc_state->pipe_bpp);
> @@ -1729,7 +1729,7 @@ void intel_ddi_set_pipe_settings(const struct
> intel_crtc_state *crtc_state)
> crtc_state->output_format != INTEL_OUTPUT_FORMAT_RGB);
>
> if (crtc_state->limited_color_range)
> - temp |= TRANS_MSA_CEA_RANGE;
> + temp |= DP_MSA_MISC_COLOR_CEA_RGB;
>
> /*
>* As per DP 1.2 spec section 2.3.4.3 while sending
> @@ -1737,8 +1737,7 @@ void intel_ddi_set_pipe_settings(const struct
> intel_crtc_state *crtc_state)
>* colorspace information.
>*/
> if (crtc_state->output_format == INTEL_OUTPUT_FORMAT_YCBCR444)
> - temp |= TRANS_MSA_SAMPLING_444 | TRANS_MSA_CLRSP_YCBCR
> |
> - TRANS_MSA_YCBCR_BT709;
> + temp |= DP_MSA_MISC_COLOR_YCBCR_444_BT709;
>
> /*
>* As per DP 1.4a spec section 2.2.4.3 [MSA Field for
> Indication
> @@ -1747,7 +1746,8 @@ void intel_ddi_set_pipe_settings(const struct
> intel_crtc_state *crtc_state)
>* indicate VSC SDP for the Pixel Encoding/Colorimetry Format.
>*/
> if (crtc_state->output_format == INTEL_OUTPUT_FORMAT_YCBCR420)
> - temp |= TRANS_MSA_USE_VSC_SDP;
> + temp |= DP_MSA_MISC_COLOR_VSC_SDP;
> +
> I915_WRITE(TRANS_MSA_MISC(cpu_transcoder), temp);
> }
>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h
> b/drivers/gpu/drm/i915/i915_reg.h
> index 35133b2ef6c9..91bf714897e5 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -9602,18 +9602,7 @@ enum skl_power_gate {
> #define _TRANSC_MSA_MISC 0x62410
> #define _TRANS_EDP_MSA_MISC 0x6f410
> #define TRANS_MSA_MISC(tran) _MMIO_TRANS2(tran, _TRANSA_MSA_MISC)
> -
> -#define TRANS_MSA_SYNC_CLK (1 << 0)
> -#define TRANS_MSA_SAMPLING_444 (2 << 1)
> -#define TRANS_MSA_CLRSP_YCBCR (1 << 3)
> -#define TRANS_MSA_YCBCR_BT709 (1 << 4)
> -#define TRANS_MSA_6_BPC (0 << 5)
> -#define TRANS_MSA_8_BPC (1 << 5)
> -#define TRANS_MSA_10_BPC(2 << 5)
> -#define TRANS_MSA_12_BPC(3 << 5)
> -#define TRANS_MSA_16_BPC(4 << 5)
> -#define TRANS_MSA_CEA_RANGE (1 << 3)
> -#define TRANS_MSA_USE_VSC_SDP (1 << 14)
> +/* See DP_MSA_MISC_* for the bit definitions */
>
> /* LCPLL Control */
> #define LCPLL_CTL_MMIO(0x130040)
The changes look good to me.
Reviewed-by: Gwan-gyeong Mun
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [Intel-gfx] [PATCH 07/12] drm/i915: Don't look at unrelated PIPECONF bits for interlaced readout
On Thu, 2019-07-18 at 17:50 +0300, Ville Syrjala wrote:
> From: Ville Syrjälä
>
> Since HSW the PIPECONF progressive vs. interlaced selection is done
> with just two bits instead of the earlier three. Let's not look at
> the
> extra bit on HSW+. Also gen2 doesn't support interlaced displays at
> all.
>
> This is actually fine as is currently because the extra bit is mbz
> (as
> are all three bits on gen2). But just to avoid mishaps in the future
> if the bits get reused let's only look at what's properly defined.
>
> Signed-off-by: Ville Syrjälä
> ---
> drivers/gpu/drm/i915/display/intel_display.c | 17 -
> 1 file changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> b/drivers/gpu/drm/i915/display/intel_display.c
> index e25b82d07d4f..ffdc350dc24a 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -8189,6 +8189,21 @@ static void intel_set_pipe_src_size(const
> struct intel_crtc_state *crtc_state)
> (crtc_state->pipe_src_h - 1));
> }
>
> +static bool intel_pipe_is_interlaced(struct intel_crtc_state
> *crtc_state)
> +{
> + struct drm_i915_private *dev_priv = to_i915(crtc_state-
> >base.crtc->dev);
> + enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
> +
> + if (IS_GEN(dev_priv, 2))
> + return false;
> +
> + if (INTEL_GEN(dev_priv) >= 9 ||
> + IS_BROADWELL(dev_priv) || IS_HASWELL(dev_priv))
> + return I915_READ(PIPECONF(cpu_transcoder)) &
> PIPECONF_INTERLACE_MASK_HSW;
> + else
> + return I915_READ(PIPECONF(cpu_transcoder)) &
> PIPECONF_INTERLACE_MASK;
> +}
> +
> static void intel_get_pipe_timings(struct intel_crtc *crtc,
> struct intel_crtc_state
> *pipe_config)
> {
> @@ -8227,7 +8242,7 @@ static void intel_get_pipe_timings(struct
> intel_crtc *crtc,
> pipe_config->base.adjusted_mode.crtc_vsync_start = (tmp &
> 0x) + 1;
> pipe_config->base.adjusted_mode.crtc_vsync_end = ((tmp >> 16) &
> 0x) + 1;
>
> - if (I915_READ(PIPECONF(cpu_transcoder)) &
> PIPECONF_INTERLACE_MASK) {
> + if (intel_pipe_is_interlaced(pipe_config)) {
> pipe_config->base.adjusted_mode.flags |=
> DRM_MODE_FLAG_INTERLACE;
> pipe_config->base.adjusted_mode.crtc_vtotal += 1;
> pipe_config->base.adjusted_mode.crtc_vblank_end += 1;
The changes look good to me.
Reviewed-by: Gwan-gyeong Mun
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [Intel-gfx] [PATCH v2 05/12] drm/i915: Never set limited_color_range=true for YCbCr output
On Thu, 2019-07-18 at 19:45 +0300, Ville Syrjala wrote:
> From: Ville Syrjälä
>
> crtc_state->limited_color_range only applies to RGB output but
> we're currently setting it even for YCbCr output. That will
> lead to conflicting MSA and PIPECONF settings which can mess
> up the image. Let's make sure limited_color_range stays unset
> with YCbCr output.
>
> Also WARN if we end up with such a bogus combination when
> programming the MSA MISC bits as it's impossible to even
> indicate quantization rangle for YCbCr via MSA MISC. YCbCr
> output is simply assumed to be limited range always. Note
> that VSC SDP does provide a mechanism for full range YCbCr,
> so in the future we may want to rethink how we compute/store
> this state.
>
> And for good measure we add the same WARN to the HDMI path.
>
> v2: s/==/!=/ in the HDMI WARN
>
> Signed-off-by: Ville Syrjälä
> ---
> drivers/gpu/drm/i915/display/intel_ddi.c | 10 +++---
> drivers/gpu/drm/i915/display/intel_dp.c | 10 ++
> drivers/gpu/drm/i915/display/intel_hdmi.c | 20 +---
> 3 files changed, 34 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c
> b/drivers/gpu/drm/i915/display/intel_ddi.c
> index 157c5851a688..7dd54f573f35 100644
> --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> @@ -1706,9 +1706,6 @@ void intel_ddi_set_pipe_settings(const struct
> intel_crtc_state *crtc_state)
>
> temp = TRANS_MSA_SYNC_CLK;
>
> - if (crtc_state->limited_color_range)
> - temp |= TRANS_MSA_CEA_RANGE;
> -
> switch (crtc_state->pipe_bpp) {
> case 18:
> temp |= TRANS_MSA_6_BPC;
> @@ -1727,6 +1724,13 @@ void intel_ddi_set_pipe_settings(const struct
> intel_crtc_state *crtc_state)
> break;
> }
>
> + /* nonsense combination */
> + WARN_ON(crtc_state->limited_color_range &&
> + crtc_state->output_format != INTEL_OUTPUT_FORMAT_RGB);
> +
> + if (crtc_state->limited_color_range)
> + temp |= TRANS_MSA_CEA_RANGE;
> +
> /*
>* As per DP 1.2 spec section 2.3.4.3 while sending
>* YCBCR 444 signals we should program MSA MISC1/0 fields with
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
> b/drivers/gpu/drm/i915/display/intel_dp.c
> index 0eb5d66f87a7..84d2724f0854 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -2126,6 +2126,16 @@ bool intel_dp_limited_color_range(const struct
> intel_crtc_state *crtc_state,
> const struct drm_display_mode *adjusted_mode =
> _state->base.adjusted_mode;
>
> + /*
> + * Our YCbCr output is always limited range.
> + * crtc_state->limited_color_range only applies to RGB,
> + * and it must never be set for YCbCr or we risk setting
> + * some conflicting bits in PIPECONF which will mess up
> + * the colors on the monitor.
> + */
> + if (crtc_state->output_format != INTEL_OUTPUT_FORMAT_RGB)
> + return false;
> +
> if (intel_conn_state->broadcast_rgb ==
> INTEL_BROADCAST_RGB_AUTO) {
> /*
>* See:
> diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c
> b/drivers/gpu/drm/i915/display/intel_hdmi.c
> index ca377ba3a15e..325abd462a46 100644
> --- a/drivers/gpu/drm/i915/display/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/display/intel_hdmi.c
> @@ -724,6 +724,10 @@ intel_hdmi_compute_avi_infoframe(struct
> intel_encoder *encoder,
>
> drm_hdmi_avi_infoframe_colorspace(frame, conn_state);
>
> + /* nonsense combination */
> + WARN_ON(crtc_state->limited_color_range &&
> + crtc_state->output_format != INTEL_OUTPUT_FORMAT_RGB);
> +
> if (crtc_state->output_format == INTEL_OUTPUT_FORMAT_RGB) {
> drm_hdmi_avi_infoframe_quant_range(frame, connector,
> adjusted_mode,
> @@ -2305,6 +2309,16 @@ static bool
> intel_hdmi_limited_color_range(const struct intel_crtc_state *crtc_s
> const struct drm_display_mode *adjusted_mode =
> _state->base.adjusted_mode;
>
> + /*
> + * Our YCbCr output is always limited range.
> + * crtc_state->limited_color_range only applies to RGB,
> + * and it must never be set for YCbCr or we risk setting
> + * some conflicting bits in PIPECONF which will mess up
> + * the colors on the monitor.
> + */
> + if (crtc_state->output_format != INTEL_OUTPUT_FORMAT_RGB)
> + return false;
> +
> if (intel_conn_state->broadcast_rgb ==
> INTEL_BROADCAST_RGB_AUTO) {
> /* See CEA-861-E - 5.1 Default Encoding Parameters */
> return crtc_state->has_hdmi_sink &&
> @@ -2341,9 +2355,6 @@ int intel_hdmi_compute_config(struct
> intel_encoder *encoder,
> if (pipe_config->has_hdmi_sink)
> pipe_config->has_infoframe = true;
>
> -
Re: [Intel-gfx] [PATCH 04/12] drm/i915: Extract intel_hdmi_limited_color_range()
On Thu, 2019-07-18 at 17:50 +0300, Ville Syrjala wrote:
> From: Ville Syrjälä
>
> Pull the code for computing the limited color range
> setting into a small helper. We'll add a bit more to it
> later.
>
> Signed-off-by: Ville Syrjälä
> ---
> drivers/gpu/drm/i915/display/intel_hdmi.c | 30 +++
>
> 1 file changed, 20 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c
> b/drivers/gpu/drm/i915/display/intel_hdmi.c
> index b8100cf21dd0..ca377ba3a15e 100644
> --- a/drivers/gpu/drm/i915/display/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/display/intel_hdmi.c
> @@ -2297,6 +2297,24 @@ intel_hdmi_ycbcr420_config(struct
> drm_connector *connector,
> return true;
> }
>
> +static bool intel_hdmi_limited_color_range(const struct
> intel_crtc_state *crtc_state,
> +const struct
> drm_connector_state *conn_state)
> +{
> + const struct intel_digital_connector_state *intel_conn_state =
> + to_intel_digital_connector_state(conn_state);
> + const struct drm_display_mode *adjusted_mode =
> + _state->base.adjusted_mode;
> +
> + if (intel_conn_state->broadcast_rgb ==
> INTEL_BROADCAST_RGB_AUTO) {
> + /* See CEA-861-E - 5.1 Default Encoding Parameters */
> + return crtc_state->has_hdmi_sink &&
> + drm_default_rgb_quant_range(adjusted_mode) ==
> + HDMI_QUANTIZATION_RANGE_LIMITED;
> + } else {
> + return intel_conn_state->broadcast_rgb ==
> INTEL_BROADCAST_RGB_LIMITED;
> + }
> +}
> +
> int intel_hdmi_compute_config(struct intel_encoder *encoder,
> struct intel_crtc_state *pipe_config,
> struct drm_connector_state *conn_state)
> @@ -2323,16 +2341,8 @@ int intel_hdmi_compute_config(struct
> intel_encoder *encoder,
> if (pipe_config->has_hdmi_sink)
> pipe_config->has_infoframe = true;
>
> - if (intel_conn_state->broadcast_rgb ==
> INTEL_BROADCAST_RGB_AUTO) {
> - /* See CEA-861-E - 5.1 Default Encoding Parameters */
> - pipe_config->limited_color_range =
> - pipe_config->has_hdmi_sink &&
> - drm_default_rgb_quant_range(adjusted_mode) ==
> - HDMI_QUANTIZATION_RANGE_LIMITED;
> - } else {
> - pipe_config->limited_color_range =
> - intel_conn_state->broadcast_rgb ==
> INTEL_BROADCAST_RGB_LIMITED;
> - }
> + pipe_config->limited_color_range =
> + intel_hdmi_limited_color_range(pipe_config,
> conn_state);
>
> if (adjusted_mode->flags & DRM_MODE_FLAG_DBLCLK) {
> pipe_config->pixel_multiplier = 2;
The changes look good to me.
Reviewed-by: Gwan-gyeong Mun
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [Intel-gfx] [PATCH 02/12] drm/i915: Fix HSW+ DP MSA YCbCr colorspace indication
On Thu, 2019-07-18 at 17:50 +0300, Ville Syrjala wrote:
> From: Ville Syrjälä
>
> Looks like we're currently setting the MSA to xvYCC BT.709 instead
> of the YCbCr BT.601 claimed by the comment. But even that comment
> is wrong since we configure the CSC matrix to BT.709.
>
> Let's remove the bogus statement from the comment and fix the
> MSA to indicate YCbCr BT.709 so that it matches the actual
> pixel data we're transmitting.
>
> Signed-off-by: Ville Syrjälä
> ---
> drivers/gpu/drm/i915/display/intel_ddi.c | 6 --
> drivers/gpu/drm/i915/i915_reg.h | 3 ++-
> 2 files changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c
> b/drivers/gpu/drm/i915/display/intel_ddi.c
> index 18bc0f2690c9..157c5851a688 100644
> --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> @@ -1730,10 +1730,12 @@ void intel_ddi_set_pipe_settings(const struct
> intel_crtc_state *crtc_state)
> /*
>* As per DP 1.2 spec section 2.3.4.3 while sending
>* YCBCR 444 signals we should program MSA MISC1/0 fields with
> - * colorspace information. The output colorspace encoding is
> BT601.
> + * colorspace information.
>*/
> if (crtc_state->output_format == INTEL_OUTPUT_FORMAT_YCBCR444)
> - temp |= TRANS_MSA_SAMPLING_444 | TRANS_MSA_CLRSP_YCBCR;
> + temp |= TRANS_MSA_SAMPLING_444 | TRANS_MSA_CLRSP_YCBCR
> |
> + TRANS_MSA_YCBCR_BT709;
> +
> /*
>* As per DP 1.4a spec section 2.2.4.3 [MSA Field for
> Indication
>* of Color Encoding Format and Content Color Gamut] while
> sending
> diff --git a/drivers/gpu/drm/i915/i915_reg.h
> b/drivers/gpu/drm/i915/i915_reg.h
> index fdd9bc01e694..35133b2ef6c9 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -9605,7 +9605,8 @@ enum skl_power_gate {
>
> #define TRANS_MSA_SYNC_CLK (1 << 0)
> #define TRANS_MSA_SAMPLING_444 (2 << 1)
> -#define TRANS_MSA_CLRSP_YCBCR (2 << 3)
> +#define TRANS_MSA_CLRSP_YCBCR (1 << 3)
> +#define TRANS_MSA_YCBCR_BT709 (1 << 4)
> #define TRANS_MSA_6_BPC (0 << 5)
> #define TRANS_MSA_8_BPC (1 << 5)
> #define TRANS_MSA_10_BPC(2 << 5)
The changes look good to me.
Reviewed-by: Gwan-gyeong Mun
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [Intel-gfx] [PATCH 01/12] drm/dp: Add definitons for MSA MISC bits
On Thu, 2019-07-18 at 17:50 +0300, Ville Syrjala wrote: > From: Ville Syrjälä > > Add definitions for the MSA (Main Stream Attribute) MISC bits. On > some hardware you can program these directly into a register. > > Signed-off-by: Ville Syrjälä > --- > include/drm/drm_dp_helper.h | 42 > + > 1 file changed, 42 insertions(+) > > diff --git a/include/drm/drm_dp_helper.h > b/include/drm/drm_dp_helper.h > index 397896b5b21a..d3f429795fea 100644 > --- a/include/drm/drm_dp_helper.h > +++ b/include/drm/drm_dp_helper.h > @@ -42,6 +42,48 @@ > * 1.2 formally includes both eDP and DPI definitions. > */ > > +/* MSA (Main Stream Attribute) MISC bits (as MISC1<<8|MISC0) */ > +#define DP_MSA_MISC_SYNC_CLOCK (1 << 0) > +#define DP_MSA_MISC_INTERLACE_VTOTAL_EVEN(1 << 8) > +#define DP_MSA_MISC_STEREO_NO_3D (0 << 9) > +#define DP_MSA_MISC_STEREO_PROG_RIGHT_EYE(1 << 9) > +#define DP_MSA_MISC_STEREO_PROG_LEFT_EYE (3 << 9) > +/* bits per component for non-RAW */ > +#define DP_MSA_MISC_6_BPC(0 << 5) > +#define DP_MSA_MISC_8_BPC(1 << 5) > +#define DP_MSA_MISC_10_BPC (2 << 5) > +#define DP_MSA_MISC_12_BPC (3 << 5) > +#define DP_MSA_MISC_16_BPC (4 << 5) > +/* bits per component for RAW */ > +#define DP_MSA_MISC_RAW_6_BPC(1 << 5) > +#define DP_MSA_MISC_RAW_7_BPC(2 << 5) > +#define DP_MSA_MISC_RAW_8_BPC(3 << 5) > +#define DP_MSA_MISC_RAW_10_BPC (4 << 5) > +#define DP_MSA_MISC_RAW_12_BPC (5 << 5) > +#define DP_MSA_MISC_RAW_14_BPC (6 << 5) > +#define DP_MSA_MISC_RAW_16_BPC (7 << 5) > +/* pixel encoding/colorimetry format */ > +#define _DP_MSA_MISC_COLOR(misc1_7, misc0_21, misc0_3, misc0_4) \ > + ((misc1_7) << 15 | (misc0_4) << 4 | (misc0_3) << 3 | > ((misc0_21) << 1)) > +#define DP_MSA_MISC_COLOR_RGB_DP_MSA_MISC_CO > LOR(0, 0, 0, 0) > +#define DP_MSA_MISC_COLOR_CEA_RGB_DP_MSA_MISC_COLOR(0, > 0, 1, 0) > +#define DP_MSA_MISC_COLOR_RGB_WIDE_FIXED _DP_MSA_MISC_COLOR(0, > 3, 0, 0) > +#define DP_MSA_MISC_COLOR_RGB_WIDE_FLOAT _DP_MSA_MISC_COLOR(0, > 3, 0, 1) > +#define DP_MSA_MISC_COLOR_Y_ONLY _DP_MSA_MISC_COLOR(1, > 0, 0, 0) > +#define DP_MSA_MISC_COLOR_RAW_DP_MSA_MISC_CO > LOR(1, 1, 0, 0) > +#define DP_MSA_MISC_COLOR_YCBCR_422_BT601_DP_MSA_MISC_COLOR(0, > 1, 1, 0) > +#define DP_MSA_MISC_COLOR_YCBCR_422_BT709_DP_MSA_MISC_COLOR(0, > 1, 1, 1) > +#define DP_MSA_MISC_COLOR_YCBCR_444_BT601_DP_MSA_MISC_COLOR(0, > 2, 1, 0) > +#define DP_MSA_MISC_COLOR_YCBCR_444_BT709_DP_MSA_MISC_COLOR(0, > 2, 1, 1) > +#define DP_MSA_MISC_COLOR_XVYCC_422_BT601_DP_MSA_MISC_COLOR(0, > 1, 0, 0) > +#define DP_MSA_MISC_COLOR_XVYCC_422_BT709_DP_MSA_MISC_COLOR(0, > 1, 0, 1) > +#define DP_MSA_MISC_COLOR_XVYCC_444_BT601_DP_MSA_MISC_COLOR(0, > 2, 0, 0) > +#define DP_MSA_MISC_COLOR_XVYCC_444_BT709_DP_MSA_MISC_COLOR(0, > 2, 0, 1) > +#define DP_MSA_MISC_COLOR_OPRGB _DP_MSA_MISC_CO > LOR(0, 0, 1, 1) > +#define DP_MSA_MISC_COLOR_DCI_P3 _DP_MSA_MISC_COLOR(0, > 3, 1, 0) > +#define DP_MSA_MISC_COLOR_COLOR_PROFILE _DP_MSA_MISC_CO > LOR(0, 3, 1, 1) > +#define DP_MSA_MISC_COLOR_VSC_SDP(1 << 14) > + > #define DP_AUX_MAX_PAYLOAD_BYTES 16 > > #define DP_AUX_I2C_WRITE 0x0 > The changes look good to me. Reviewed-by: Gwan-gyeong Mun > -- > 2.21.0 > > ___ > Intel-gfx mailing list > intel-...@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v7 3/7] drm: Add DisplayPort colorspace property
On Fri, 2019-09-13 at 22:13 +0300, Ville Syrjälä wrote:
> On Thu, Sep 12, 2019 at 02:33:34PM +0300, Gwan-gyeong Mun wrote:
> > Because between HDMI and DP have different colorspaces, it renames
> > drm_mode_create_colorspace_property() function to
> > drm_mode_create_hdmi_colorspace_property() function for HDMI
> > connector.
> > And it adds drm_mode_create_dp_colorspace_property() function for
> > creating
> > of DP colorspace property.
> > In order to apply changed and added drm api, i915 driver has
> > channged.
> >
> > v3: Addressed review comments from Ville
> > - Add new colorimetry options for DP 1.4a spec.
> > - Separate set of colorimetry enum values for DP.
> > v4: Add additional comments to struct drm_prop_enum_list.
> > Polishing an enum string of struct drm_prop_enum_list
> > v5: Change definitions of DRM_MODE_COLORIMETRYs to follow HDMI
> > prefix and
> > DP abbreviations.
> > Add missed variables on dp_colorspaces.
> > Fix typo. [Uma]
> > v6: Addressed review comments from Ilia and Ville
> >- Split drm_mode_create_colorspace_property() to DP and HDMI
> > connector.
> > v7: Fix typo [Jani Saarinen]
> > Fix white space.
> >
> > Signed-off-by: Gwan-gyeong Mun
> > Reviewed-by: Uma Shankar
> > ---
> > drivers/gpu/drm/drm_connector.c | 110
> > +++---
> > .../gpu/drm/i915/display/intel_connector.c| 21 +++-
> > include/drm/drm_connector.h | 11 +-
> > 3 files changed, 121 insertions(+), 21 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_connector.c
> > b/drivers/gpu/drm/drm_connector.c
> > index 4c766624b20d..656f72c1b3d7 100644
> > --- a/drivers/gpu/drm/drm_connector.c
> > +++ b/drivers/gpu/drm/drm_connector.c
> > @@ -882,6 +882,47 @@ static const struct drm_prop_enum_list
> > hdmi_colorspaces[] = {
> > { DRM_MODE_COLORIMETRY_DCI_P3_RGB_THEATER, "DCI-P3_RGB_Theater"
> > },
> > };
> >
> > +/*
> > + * As per DP 1.4a spec, 2.2.5.7.5 VSC SDP Payload for Pixel
> > Encoding/Colorimetry
> > + * Format Table 2-120
> > + */
> > +static const struct drm_prop_enum_list dp_colorspaces[] = {
> > + /* For Default case, driver will set the colorspace */
> > + { DRM_MODE_COLORIMETRY_DEFAULT, "Default" },
> > + /* Colorimetry based on sRGB (IEC 61966-2-1) */
> > + { DRM_MODE_COLORIMETRY_CEA_RGB, "CEA_RGB" },
>
> We already have other mechanism for the CEA vs. IT RGB.
> I don't think we want to add another one. So I'd drop this.
>
> > + { DRM_MODE_COLORIMETRY_RGB_WIDE_FIXED,
> > "RGB_Wide_Gamut_Fixed_Point" },
> > + /* Colorimetry based on scRGB (IEC 61966-2-2) */
> > + { DRM_MODE_COLORIMETRY_RGB_WIDE_FLOAT,
> > "RGB_Wide_Gamut_Floating_Point" },
> > + /* Colorimetry based on IEC 61966-2-5 */
> > + { DRM_MODE_COLORIMETRY_OPRGB, "opRGB" },
> > + /* Colorimetry based on SMPTE RP 431-2 */
> > + { DRM_MODE_COLORIMETRY_DCI_P3_RGB_D65, "DCI-P3_RGB_D65" },
> > + { DRM_MODE_COLORIMETRY_RGB_CUSTOM_COLOR_PROFILE,
> > "RGB_Custom_Color_Profile" },
>
> I'd also drop this since we have no way to supply the profile anyway.
>
> > + /* Colorimetry based on ITU-R BT.2020 */
> > + { DRM_MODE_COLORIMETRY_BT2020_RGB, "BT2020_RGB" },
> > + { DRM_MODE_COLORIMETRY_BT601_YCC, "BT601_YCC" },
> > + { DRM_MODE_COLORIMETRY_BT709_YCC, "BT709_YCC" },
> > + /* Standard Definition Colorimetry based on IEC 61966-2-4 */
> > + { DRM_MODE_COLORIMETRY_XVYCC_601, "XVYCC_601" },
> > + /* High Definition Colorimetry based on IEC 61966-2-4 */
> > + { DRM_MODE_COLORIMETRY_XVYCC_709, "XVYCC_709" },
> > + /* Colorimetry based on IEC 61966-2-1/Amendment 1 */
> > + { DRM_MODE_COLORIMETRY_SYCC_601, "SYCC_601" },
> > + /* Colorimetry based on IEC 61966-2-5 [33] */
> > + { DRM_MODE_COLORIMETRY_OPYCC_601, "opYCC_601" },
> > + /* Colorimetry based on ITU-R BT.2020 */
> > + { DRM_MODE_COLORIMETRY_BT2020_CYCC, "BT2020_CYCC" },
> > + /* Colorimetry based on ITU-R BT.2020 */
> > + { DRM_MODE_COLORIMETRY_BT2020_YCC, "BT2020_YCC" },
> > + /*
> > +* Colorimetry based on Digital Imaging and Communications in
> > Medicine
> > +* (DICOM) Part 14: Grayscale Standard Display Function
> > +*/
> > + { DRM_MODE_COLORIMETRY_Y_ONLY_DICOM_P14_GRAYSCALE,
> > "Y_ONLY_DICOM_Part_14_Grayscale" },
> > + { DRM_MODE_COLORIMETRY_RAW_CUSTOM_COLOR_PROFILE,
> > "Raw_Custom_Color_Profile" },
>
> And these last two seem rather esoteric as well. I don't think any
> driver supports RAW/Y stuff so I'd drop them too.
>
Ville, thank you for reviewing and check in detail.
I'll drop them which you commented.
> > +};
> > +
> > /**
> > * DOC: standard connector properties
> > *
> > @@ -1674,7 +1715,6 @@
> > EXPORT_SYMBOL(drm_mode_create_aspect_ratio_property);
> > * DOC: standard connector properties
> > *
> > * Colorspace:
> > - * drm_mode_create_colorspace_property - create colorspace
> > property
> > * This property helps select a suitable colorspace based on
> > the sink
> > * capability.
Re: [PATCH v4 3/7] drm: Add DisplayPort colorspace property
On Sat, 2019-09-07 at 21:43 -0400, Ilia Mirkin wrote: > On Sat, Sep 7, 2019 at 7:20 PM Mun, Gwan-gyeong > wrote: > > On Fri, 2019-09-06 at 09:24 -0400, Ilia Mirkin wrote: > > > On Fri, Sep 6, 2019 at 7:43 AM Ville Syrjälä > > > wrote: > > > > On Fri, Sep 06, 2019 at 11:31:55AM +, Shankar, Uma wrote: > > > > > > -Original Message- > > > > > > From: Ilia Mirkin > > > > > > Sent: Tuesday, September 3, 2019 6:12 PM > > > > > > To: Mun, Gwan-gyeong > > > > > > Cc: Intel Graphics Development < > > > > > > intel-...@lists.freedesktop.org > > > > > > > ; Shankar, Uma > > > > > > ; dri-devel < > > > > > > dri-devel@lists.freedesktop.org> > > > > > > Subject: Re: [PATCH v4 3/7] drm: Add DisplayPort colorspace > > > > > > property > > > > > > > > > > > > So how would this work with a DP++ connector? Should it > > > > > > list > > > > > > the HDMI or DP > > > > > > properties? Or do we need a custom property checker which > > > > > > is > > > > > > aware of what is > > > > > > currently plugged in to validate the values? > > > > > > > > > > AFAIU For DP++ cases, we detect what kind of sink its driving > > > > > DP > > > > > or HDMI (with a passive dongle). > > > > > Based on the type of sink detected, we should expose DP or > > > > > HDMI > > > > > colorspaces to userspace. > > > > > > > > For i915 DP connector always drives DP mode, HDMI connector > > > > always > > > > drives > > > > HDMI mode, even when the physical connector is DP++. > > > > > > Right, i915 creates 2 connectors, while nouveau, radeon, and > > > amdgpu > > > create 1 connector (not sure about other drivers) for a single > > > physical DP++ socket. Since we supply the list of valid values at > > > the > > > time of creating the connector, we can't know at that point > > > whether > > > in > > > the future a HDMI or DP will be plugged into it. > > > > > > -ilia > > Ilia, does it mean that the drm_connector type is > > DRM_MODE_CONNECTOR_DisplayPort and protocol is DP++ mode? > > That is correct. The connector type is "DisplayPort" in such a case. > > Cheers, > > -ilia For now drm_mode_create_colorspace_property() is only used for i915. IMHO, when other drivers ( nouveau, radeon, and amdgpu ) are ready for using of drm_mode_create_colorspace_property(), what about do we add a variable which can identify DP++ and DP to drm_connector? And when the drivers (nouveau, radeon, and amdgpu) detect the current protocol, the drivers will set the variable. Br, - G.G. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v4 3/7] drm: Add DisplayPort colorspace property
On Mon, 2019-09-09 at 13:25 +0300, Ville Syrjälä wrote: > On Sat, Sep 07, 2019 at 11:19:55PM +0000, Mun, Gwan-gyeong wrote: > > On Fri, 2019-09-06 at 09:24 -0400, Ilia Mirkin wrote: > > > On Fri, Sep 6, 2019 at 7:43 AM Ville Syrjälä > > > wrote: > > > > On Fri, Sep 06, 2019 at 11:31:55AM +, Shankar, Uma wrote: > > > > > > -Original Message- > > > > > > From: Ilia Mirkin > > > > > > Sent: Tuesday, September 3, 2019 6:12 PM > > > > > > To: Mun, Gwan-gyeong > > > > > > Cc: Intel Graphics Development < > > > > > > intel-...@lists.freedesktop.org > > > > > > > ; Shankar, Uma > > > > > > ; dri-devel < > > > > > > dri-devel@lists.freedesktop.org> > > > > > > Subject: Re: [PATCH v4 3/7] drm: Add DisplayPort colorspace > > > > > > property > > > > > > > > > > > > So how would this work with a DP++ connector? Should it > > > > > > list > > > > > > the HDMI or DP > > > > > > properties? Or do we need a custom property checker which > > > > > > is > > > > > > aware of what is > > > > > > currently plugged in to validate the values? > > > > > > > > > > AFAIU For DP++ cases, we detect what kind of sink its driving > > > > > DP > > > > > or HDMI (with a passive dongle). > > > > > Based on the type of sink detected, we should expose DP or > > > > > HDMI > > > > > colorspaces to userspace. > > > > > > > > For i915 DP connector always drives DP mode, HDMI connector > > > > always > > > > drives > > > > HDMI mode, even when the physical connector is DP++. > > > > > > Right, i915 creates 2 connectors, while nouveau, radeon, and > > > amdgpu > > > create 1 connector (not sure about other drivers) for a single > > > physical DP++ socket. Since we supply the list of valid values at > > > the > > > time of creating the connector, we can't know at that point > > > whether > > > in > > > the future a HDMI or DP will be plugged into it. > > > > > > -ilia > > Ilia, does it mean that the drm_connector type is > > DRM_MODE_CONNECTOR_DisplayPort and protocol is DP++ mode? > > > > And Ville and Uma, when we are useing dp active dongle (DP to HDMI > > dongle and DP branch device is HDMI) should we expose HDMI > > colorspace? > > We still set it up via DP MSA/VSC no? In that case it should follow > the > DP spec I think. LSPCON is probably different because we manually Yes, I agree too. - G.G. > generate > the AVI infoframe for it. But I'm not sure how we're going to > reconcile > that with the DP stuff we also set up for it. > ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v4 3/7] drm: Add DisplayPort colorspace property
On Fri, 2019-09-06 at 09:24 -0400, Ilia Mirkin wrote: > On Fri, Sep 6, 2019 at 7:43 AM Ville Syrjälä > wrote: > > On Fri, Sep 06, 2019 at 11:31:55AM +, Shankar, Uma wrote: > > > > > > > -Original Message- > > > > From: Ilia Mirkin > > > > Sent: Tuesday, September 3, 2019 6:12 PM > > > > To: Mun, Gwan-gyeong > > > > Cc: Intel Graphics Development > > > >; Shankar, Uma > > > > ; dri-devel < > > > > dri-devel@lists.freedesktop.org> > > > > Subject: Re: [PATCH v4 3/7] drm: Add DisplayPort colorspace > > > > property > > > > > > > > So how would this work with a DP++ connector? Should it list > > > > the HDMI or DP > > > > properties? Or do we need a custom property checker which is > > > > aware of what is > > > > currently plugged in to validate the values? > > > > > > AFAIU For DP++ cases, we detect what kind of sink its driving DP > > > or HDMI (with a passive dongle). > > > Based on the type of sink detected, we should expose DP or HDMI > > > colorspaces to userspace. > > > > For i915 DP connector always drives DP mode, HDMI connector always > > drives > > HDMI mode, even when the physical connector is DP++. > > Right, i915 creates 2 connectors, while nouveau, radeon, and amdgpu > create 1 connector (not sure about other drivers) for a single > physical DP++ socket. Since we supply the list of valid values at the > time of creating the connector, we can't know at that point whether > in > the future a HDMI or DP will be plugged into it. > > -ilia Ilia, does it mean that the drm_connector type is DRM_MODE_CONNECTOR_DisplayPort and protocol is DP++ mode? And Ville and Uma, when we are useing dp active dongle (DP to HDMI dongle and DP branch device is HDMI) should we expose HDMI colorspace? -G.G. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 5/6] drm/i915/dp: Program an Infoframe SDP Header and DB for HDR Static Metadata
On Tue, 2019-08-27 at 01:14 +0530, Shankar, Uma wrote:
> > -Original Message-
> > From: Mun, Gwan-gyeong
> > Sent: Friday, August 23, 2019 3:23 PM
> > To: intel-...@lists.freedesktop.org
> > Cc: dri-devel@lists.freedesktop.org; Shankar, Uma <
> > uma.shan...@intel.com>;
> > Sharma, Shashank
> > Subject: [PATCH v2 5/6] drm/i915/dp: Program an Infoframe SDP
> > Header and DB for
> > HDR Static Metadata
> >
> > Function intel_dp_setup_hdr_metadata_infoframe_sdp handles
> > Infoframe SDP
> > header and data block setup for HDR Static Metadata. It enables
> > writing of HDR
> > metadata infoframe SDP to panel. Support for HDR video was
> > introduced in
> > DisplayPort 1.4. It implements the CTA-861-G standard for transport
> > of static HDR
> > metadata. The HDR Metadata will be provided by userspace
> > compositors, based on
> > blending policies and passed to the driver through a blob property.
> >
> > Because each of GEN11 and prior GEN11 have different register size
> > for HDR
> > Metadata Infoframe SDP packet, it adds and uses different register
> > size.
> >
> > Setup Infoframe SDP header and data block in function
> > intel_dp_setup_hdr_metadata_infoframe_sdp for HDR Static Metadata
> > as per dp 1.4
> > spec and CTA-861-F spec.
> > As per DP 1.4 spec, 2.2.2.5 SDP Formats. It enables Dynamic Range
> > and Mastering
> > Infoframe for HDR content, which is defined in CTA-861-F spec.
> > According to DP 1.4 spec and CEA-861-F spec Table 5, in order to
> > transmit static HDR
> > metadata, we have to use Non-audio INFOFRAME SDP v1.3.
> >
> > ++---+
> > > [ Packet Type Value ] | [ Packet Type ] |
> > ++---+
> > > 80h + Non-audio INFOFRAME Type | CEA-861-F Non-audio INFOFRAME |
> > ++---+
> > > [Transmission Timing] |
> > ++
> > > As per CEA-861-F for INFOFRAME, including CEA-861.3 within |
> > > which Dynamic Range and Mastering INFOFRAME are defined|
> > ++
> >
> > v2: Add a missed blank line after function declaration.
> >
> > Signed-off-by: Gwan-gyeong Mun
> > ---
> > drivers/gpu/drm/i915/display/intel_ddi.c | 1 +
> > drivers/gpu/drm/i915/display/intel_dp.c | 91
> >
> > drivers/gpu/drm/i915/display/intel_dp.h | 3 +
> > drivers/gpu/drm/i915/i915_reg.h | 1 +
> > 4 files changed, 96 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c
> > b/drivers/gpu/drm/i915/display/intel_ddi.c
> > index 5c42b58c1c2f..9f5bea941bcd 100644
> > --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> > @@ -3478,6 +3478,7 @@ static void intel_enable_ddi_dp(struct
> > intel_encoder
> > *encoder,
> > intel_edp_backlight_on(crtc_state, conn_state);
> > intel_psr_enable(intel_dp, crtc_state);
> > intel_dp_vsc_enable(intel_dp, crtc_state, conn_state);
> > + intel_dp_hdr_metadata_enable(intel_dp, crtc_state, conn_state);
> > intel_edp_drrs_enable(intel_dp, crtc_state);
> >
> > if (crtc_state->has_audio)
> > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
> > b/drivers/gpu/drm/i915/display/intel_dp.c
> > index 7218e159f55d..00da8221eaba 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > @@ -4554,6 +4554,85 @@ intel_dp_setup_vsc_sdp(struct intel_dp
> > *intel_dp,
> > crtc_state, DP_SDP_VSC, _sdp,
> > sizeof(vsc_sdp)); }
> >
> > +static int
> > +intel_dp_setup_hdr_metadata_infoframe_sdp(struct intel_dp
> > *intel_dp,
> > + const struct intel_crtc_state
> > *crtc_state,
> > + const struct
> > drm_connector_state
>
> The return value is not handled, you may convert it as void.
>
Okay, I'll remove the return values which is not handled from
intel_dp_setup_hdr_metadata_infoframe_sdp().
> > *conn_state) {
> > + struct intel_digital_port *intel_dig_port =
> > dp_to_dig_port(intel_dp);
> > + struct drm_i915_private *dev_priv = to_i915(intel_dig_port-
>
Re: [Intel-gfx] [PATCH v2 3/6] drm: Add DisplayPort colorspace property
On Mon, 2019-09-02 at 17:44 +0300, Ville Syrjälä wrote:
> On Fri, Aug 23, 2019 at 12:52:29PM +0300, Gwan-gyeong Mun wrote:
> > In order to use colorspace property to Display Port connectors, it
> > extends
> > DRM_MODE_CONNECTOR_DisplayPort connector_type on
> > drm_mode_create_colorspace_property function.
> >
> > Signed-off-by: Gwan-gyeong Mun
> > ---
> > drivers/gpu/drm/drm_connector.c | 4 +++-
> > 1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/drm_connector.c
> > b/drivers/gpu/drm/drm_connector.c
> > index 4c766624b20d..655ada9d4c16 100644
> > --- a/drivers/gpu/drm/drm_connector.c
> > +++ b/drivers/gpu/drm/drm_connector.c
> > @@ -1703,7 +1703,9 @@ int
> > drm_mode_create_colorspace_property(struct drm_connector
> > *connector)
> > struct drm_property *prop;
> >
> > if (connector->connector_type == DRM_MODE_CONNECTOR_HDMIA ||
> > - connector->connector_type == DRM_MODE_CONNECTOR_HDMIB) {
> > + connector->connector_type == DRM_MODE_CONNECTOR_HDMIB ||
> > + connector->connector_type == DRM_MODE_CONNECTOR_DisplayPort
> > ||
> > + connector->connector_type == DRM_MODE_CONNECTOR_eDP) {
>
> We don't need a separate set of enum values for DP?
>
I checked DP 1.4a spec, 2.2.5.7.5 VSC SDP Payload for Pixel
Encoding/Colorimetry again,
Followed your comments, the spec requires more new colorimetry options
for DP 1.4a colorimetry format.
I'll add missed colorimetry options and will separate set of
colorimetry enum values for DP.
> > prop = drm_property_create_enum(dev,
> > DRM_MODE_PROP_ENUM,
> > "Colorspace",
> > hdmi_colorspaces,
> > --
> > 2.22.0
> >
> > ___
> > Intel-gfx mailing list
> > intel-...@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [Intel-gfx] [PATCH v2 2/6] drm/i915/dp: Add support of BT.2020 Colorimetry to DP MSA
On Mon, 2019-09-02 at 17:43 +0300, Ville Syrjälä wrote:
> On Fri, Aug 23, 2019 at 12:52:28PM +0300, Gwan-gyeong Mun wrote:
> > When BT.2020 Colorimetry output is used for DP, we should program
> > BT.2020
> > Colorimetry to MSA and VSC SDP. It adds output_colorspace to
> > intel_crtc_state struct as a place holder of pipe's output
> > colorspace.
> > In order to distinguish needed colorimetry for VSC SDP, it adds
> > intel_dp_needs_vsc_colorimetry function.
> > If the output colorspace requires vsc sdp or output format is YCbCr
> > 4:2:0,
> > it uses MSA with VSC SDP.
> >
> > As per DP 1.4a spec section 2.2.4.3 [MSA Field for Indication of
> > Color Encoding Format and Content Color Gamut] while sending
> > BT.2020 Colorimetry signals we should program MSA MISC1 fields
> > which
> > indicate VSC SDP for the Pixel Encoding/Colorimetry Format.
> >
> > v2: Remove useless parentheses
> >
> > Signed-off-by: Gwan-gyeong Mun
> > ---
> > drivers/gpu/drm/i915/display/intel_ddi.c | 8 +++---
> > .../drm/i915/display/intel_display_types.h| 3 +++
> > drivers/gpu/drm/i915/display/intel_dp.c | 25
> > ++-
> > drivers/gpu/drm/i915/display/intel_dp.h | 1 +
> > 4 files changed, 33 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c
> > b/drivers/gpu/drm/i915/display/intel_ddi.c
> > index 4f7ea0a35976..5c42b58c1c2f 100644
> > --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> > @@ -1737,11 +1737,13 @@ void intel_ddi_set_pipe_settings(const
> > struct intel_crtc_state *crtc_state)
> > /*
> > * As per DP 1.4a spec section 2.2.4.3 [MSA Field for
> > Indication
> > * of Color Encoding Format and Content Color Gamut] while
> > sending
> > -* YCBCR 420 signals we should program MSA MISC1 fields which
> > -* indicate VSC SDP for the Pixel Encoding/Colorimetry Format.
> > +* YCBCR 420, HDR BT.2020 signals we should program MSA MISC1
> > fields
> > +* which indicate VSC SDP for the Pixel Encoding/Colorimetry
> > Format.
> > */
> > - if (crtc_state->output_format == INTEL_OUTPUT_FORMAT_YCBCR420)
> > + if (crtc_state->output_format == INTEL_OUTPUT_FORMAT_YCBCR420
> > ||
> > + intel_dp_needs_vsc_colorimetry(crtc_state-
> > >output_colorspace))
> > temp |= TRANS_MSA_USE_VSC_SDP;
> > +
> > I915_WRITE(TRANS_MSA_MISC(cpu_transcoder), temp);
> > }
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h
> > b/drivers/gpu/drm/i915/display/intel_display_types.h
> > index 449abaea619f..9845abcf6f29 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> > @@ -964,6 +964,9 @@ struct intel_crtc_state {
> > /* Output format RGB/YCBCR etc */
> > enum intel_output_format output_format;
> >
> > + /* Output colorspace sRGB/BT.2020 etc */
> > + u32 output_colorspace;
> > +
> > /* Output down scaling is done in LSPCON device */
> > bool lspcon_downsampling;
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
> > b/drivers/gpu/drm/i915/display/intel_dp.c
> > index 55d5ab97061c..295d5ed2be96 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > @@ -2164,6 +2164,8 @@ intel_dp_compute_config(struct intel_encoder
> > *encoder,
> > pipe_config->has_pch_encoder = true;
> >
> > pipe_config->output_format = INTEL_OUTPUT_FORMAT_RGB;
> > + pipe_config->output_colorspace = intel_conn_state-
> > >base.colorspace;
> > +
> > if (lspcon->active)
> > lspcon_ycbcr420_config(_connector->base,
> > pipe_config);
> > else
> > @@ -4408,6 +4410,26 @@ u8 intel_dp_dsc_get_slice_count(struct
> > intel_dp *intel_dp,
> > return 0;
> > }
> >
> > +bool
> > +intel_dp_needs_vsc_colorimetry(u32 colorspace)
>
> I would pass the entire crtc state here so you handle handle the
> 4:2:0
> case here as well.
>
Okay, I'll pass the entire intel_crtc_state stucture value to
intel_dp_needs_vsc_colorimetry() for checking output format (YCbCr
4:2:0) and output colorspace here. And I'll change the fuction name to
intel_dp_needs_vsc_sdp().
> > +{
> > + bool ret = false;
>
> Pointless variable.
>
I'll remove pointless variables.
> > +
> > + switch (colorspace) {
> > + case DRM_MODE_COLORIMETRY_SYCC_601:
> > + case DRM_MODE_COLORIMETRY_OPYCC_601:
> > + case DRM_MODE_COLORIMETRY_BT2020_YCC:
> > + case DRM_MODE_COLORIMETRY_BT2020_RGB:
> > + case DRM_MODE_COLORIMETRY_BT2020_CYCC:
> > + ret = true;
> > + break;
> > + default:
> > + break;
> > + }
> > +
> > + return ret;
> > +}
> > +
> > static void
> > intel_dp_setup_vsc_sdp(struct intel_dp *intel_dp,
> >const struct intel_crtc_state *crtc_state,
> > @@ -4536,7 +4558,8 @@ void intel_dp_vsc_enable(struct intel_dp
> > *intel_dp,
> > const
Re: [Intel-gfx] [PATCH v8 2/6] drm: Rename struct edp_vsc_psr to struct dp_sdp
On Tue, 2019-05-21 at 13:14 +0300, Laurent Pinchart wrote:
> Hello Jani,
>
> On Tue, May 21, 2019 at 09:44:04AM +0300, Jani Nikula wrote:
> > On Mon, 20 May 2019, Gwan-gyeong Mun
> > wrote:
> > > VSC SDP Payload for PSR is one of data block type of SDP
> > > (Secondaray Data
> > > Packet). In order to generalize SDP packet structure name, it
> > > renames
> > > struct edp_vsc_psr to struct dp_sdp. And each SDP data blocks
> > > have
> > > different usages, each SDP type has different reserved data
> > > blocks and
> > > Video_Stream_Configuration Extension VESA SDP might use all of
> > > Data Blocks
> > > as Extended INFORFRAME Data Byte. so it makes Data Block
> > > variables as
> > > array type. And it adds comments of details of DB of VSC SDP
> > > Payload
> > > for Pixel Encoding/Colorimetry Format. This comments follows DP
> > > 1.4a spec,
> > > section 2.2.5.7.5, chapter "VSC SDP Payload for Pixel
> > > Encoding/Colorimetry
> > > Format".
> > >
> > > v7: Addressed review comments from Ville.
> > >
> > > Cc: Ville Syrjälä
> > > Signed-off-by: Gwan-gyeong Mun
> > > Reviewed-by: Maarten Lankhorst > > >
> >
> > Andrzej, Laurent -
> >
> > Seven versions of the patch and looks like we've failed to loop you
> > in
> > on this. Sorry. May I have your ack on the patch please?
>
> Please see below for one comment.
>
> > Is it too much to ask to have this merged via drm-intel along with
> > the
> > rest of the series?
>
> Provided the potential conflicts are handled I have no issue with
> that.
>
Andrzej, Laurent -
I am Sorry that I missed you on previous loops.
And thank you for reviewing the series.
Jani,
Thank you for asking the series to Andrzej and Laurent.
> > > ---
> > > .../drm/bridge/analogix/analogix_dp_core.c| 12 +++
> > > .../drm/bridge/analogix/analogix_dp_core.h| 2 +-
> > > .../gpu/drm/bridge/analogix/analogix_dp_reg.c | 10 +++---
> > > drivers/gpu/drm/i915/intel_psr.c | 2 +-
> > > include/drm/drm_dp_helper.h | 33
> > > +--
> > > 5 files changed, 36 insertions(+), 23 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> > > b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> > > index 225f5e5dd69b..d1c2659d0cce 100644
> > > --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> > > +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> > > @@ -115,7 +115,7 @@ EXPORT_SYMBOL_GPL(analogix_dp_psr_enabled);
> > >
> > > int analogix_dp_enable_psr(struct analogix_dp_device *dp)
> > > {
> > > - struct edp_vsc_psr psr_vsc;
> > > + struct dp_sdp psr_vsc;
> > >
> > > if (!dp->psr_enable)
> > > return 0;
> > > @@ -127,8 +127,8 @@ int analogix_dp_enable_psr(struct
> > > analogix_dp_device *dp)
> > > psr_vsc.sdp_header.HB2 = 0x2;
> > > psr_vsc.sdp_header.HB3 = 0x8;
> > >
> > > - psr_vsc.DB0 = 0;
> > > - psr_vsc.DB1 = EDP_VSC_PSR_STATE_ACTIVE |
> > > EDP_VSC_PSR_CRC_VALUES_VALID;
> > > + psr_vsc.DB[0] = 0;
> > > + psr_vsc.DB[1] = EDP_VSC_PSR_STATE_ACTIVE |
> > > EDP_VSC_PSR_CRC_VALUES_VALID;
> > >
> > > return analogix_dp_send_psr_spd(dp, _vsc, true);
> > > }
> > > @@ -136,7 +136,7 @@ EXPORT_SYMBOL_GPL(analogix_dp_enable_psr);
> > >
> > > int analogix_dp_disable_psr(struct analogix_dp_device *dp)
> > > {
> > > - struct edp_vsc_psr psr_vsc;
> > > + struct dp_sdp psr_vsc;
> > > int ret;
> > >
> > > if (!dp->psr_enable)
> > > @@ -149,8 +149,8 @@ int analogix_dp_disable_psr(struct
> > > analogix_dp_device *dp)
> > > psr_vsc.sdp_header.HB2 = 0x2;
> > > psr_vsc.sdp_header.HB3 = 0x8;
> > >
> > > - psr_vsc.DB0 = 0;
> > > - psr_vsc.DB1 = 0;
> > > + psr_vsc.DB[0] = 0;
> > > + psr_vsc.DB[1] = 0;
> > >
> > > ret = drm_dp_dpcd_writeb(>aux, DP_SET_POWER,
> > > DP_SET_POWER_D0);
> > > if (ret != 1) {
> > > diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
> > > b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
> > > index 769255dc6e99..3e5fe90edf71 100644
> > > --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
> > > +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
> > > @@ -254,7 +254,7 @@ void analogix_dp_enable_scrambling(struct
> > > analogix_dp_device *dp);
> > > void analogix_dp_disable_scrambling(struct analogix_dp_device
> > > *dp);
> > > void analogix_dp_enable_psr_crc(struct analogix_dp_device *dp);
> > > int analogix_dp_send_psr_spd(struct analogix_dp_device *dp,
> > > - struct edp_vsc_psr *vsc, bool blocking);
> > > + struct dp_sdp *vsc, bool blocking);
> > > ssize_t analogix_dp_transfer(struct analogix_dp_device *dp,
> > >struct drm_dp_aux_msg *msg);
> > >
> > > diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
> > > b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
> > > index a5f2763d72e4..f591810ef1be 100644
> > > --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
> > > +++
Re: [PATCH v7 3/6] drm/i915/dp: Program VSC Header and DB for Pixel Encoding/Colorimetry Format
On Fri, 2019-05-17 at 15:36 +0200, Maarten Lankhorst wrote:
> Op 10-05-2019 om 03:53 schreef Gwan-gyeong Mun:
> > Function intel_pixel_encoding_setup_vsc handles vsc header and data
> > block
> > setup for pixel encoding / colorimetry format.
> >
> > Setup VSC header and data block in function
> > intel_pixel_encoding_setup_vsc
> > for pixel encoding / colorimetry format as per dp 1.4a spec,
> > section 2.2.5.7.1, table 2-119: VSC SDP Header Bytes, section
> > 2.2.5.7.5,
> > table 2-120:VSC SDP Payload for DB16 through DB18.
> >
> > v2:
> > Minor style fix. [Maarten]
> > Refer to commit ids instead of patchwork. [Maarten]
> >
> > v6: Rebase
> >
> > v7:
> > Rebase and addressed review comments from Ville.
> > Use a structure initializer instead of memset().
> > Fix non-standard comment format.
> > Remove a referring to specific commit.
> > Add a setting of dynamic range bit to vsc_sdp.DB17.
> > Add a setting of bpc which is based on pipe_bpp.
> > Remove duplicated checking of connector's ycbcr_420_allowed from
> > intel_pixel_encoding_setup_vsc(). It is already checked from
> > intel_dp_ycbcr420_config().
> > Remove comments for VSC_SDP_EXTENSION_FOR_COLORIMETRY_SUPPORTED.
> > It is
> > already implemented on intel_dp_get_colorimetry_status().
> >
> > Cc: Maarten Lankhorst
> > Cc: Ville Syrjälä
> > Signed-off-by: Gwan-gyeong Mun
> > ---
> > drivers/gpu/drm/i915/intel_ddi.c | 1 +
> > drivers/gpu/drm/i915/intel_dp.c | 90
> >
> > drivers/gpu/drm/i915/intel_drv.h | 2 +
> > 3 files changed, 93 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_ddi.c
> > b/drivers/gpu/drm/i915/intel_ddi.c
> > index cd5277d98b03..2f1688ea5a2c 100644
> > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > @@ -3391,6 +3391,7 @@ static void intel_enable_ddi_dp(struct
> > intel_encoder *encoder,
> >
> > intel_edp_backlight_on(crtc_state, conn_state);
> > intel_psr_enable(intel_dp, crtc_state);
> > + intel_dp_ycbcr_420_enable(intel_dp, crtc_state);
> > intel_edp_drrs_enable(intel_dp, crtc_state);
> >
> > if (crtc_state->has_audio)
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > b/drivers/gpu/drm/i915/intel_dp.c
> > index 9f219f8f0c71..a3fd279a5c52 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -4407,6 +4407,96 @@ u8 intel_dp_dsc_get_slice_count(struct
> > intel_dp *intel_dp,
> > return 0;
> > }
> >
> > +static void
> > +intel_pixel_encoding_setup_vsc(struct intel_dp *intel_dp,
> > + const struct intel_crtc_state
> > *crtc_state)
> > +{
> > + struct intel_digital_port *intel_dig_port =
> > dp_to_dig_port(intel_dp);
> > + struct dp_sdp vsc_sdp = {};
> > +
> > + /* Prepare VSC Header for SU as per DP 1.4a spec, Table 2-119
> > */
> > + vsc_sdp.sdp_header.HB0 = 0;
> > + vsc_sdp.sdp_header.HB1 = 0x7;
> > +
> > + /*
> > +* VSC SDP supporting 3D stereo, PSR2, and Pixel Encoding/
> > +* Colorimetry Format indication.
> > +*/
> > + vsc_sdp.sdp_header.HB2 = 0x5;
> > +
> > + /*
> > +* VSC SDP supporting 3D stereo, + PSR2, + Pixel Encoding/
> > +* Colorimetry Format indication (HB2 = 05h).
> > +*/
> > + vsc_sdp.sdp_header.HB3 = 0x13;
> > +
> > + /*
> > +* YCbCr 420 = 3h DB16[7:4] ITU-R BT.601 = 0h, ITU-R BT.709 =
> > 1h
> > +* DB16[3:0] DP 1.4a spec, Table 2-120
> > +*/
> > + vsc_sdp.DB[16] = 0x3 << 4; /* 0x3 << 4 , YCbCr 420*/
> > + /* RGB->YCBCR color conversion uses the BT.709 color space. */
> > + vsc_sdp.DB[16] |= 0x1; /* 0x1, ITU-R BT.709 */
> > +
> > + /*
> > +* For pixel encoding formats YCbCr444, YCbCr422, YCbCr420, and
> > Y Only,
> > +* the following Component Bit Depth values are defined:
> > +* 001b = 8bpc.
> > +* 010b = 10bpc.
> > +* 011b = 12bpc.
> > +* 100b = 16bpc.
> > +*/
> > + switch (crtc_state->pipe_bpp) {
> > + case 24: /* 8bpc */
> > + vsc_sdp.DB[17] = 0x1;
> > + break;
> > + case 30: /* 10bpc */
> > + vsc_sdp.DB[17] = 0x2;
> > + break;
> > + case 36: /* 12bpc */
> > + vsc_sdp.DB[17] = 0x3;
> > + break;
> > + case 48: /* 16bpc */
> > + vsc_sdp.DB[17] = 0x4;
> > + break;
> > + default:
> > + DRM_DEBUG_KMS("Invalid bpp value '%d'\n", crtc_state-
> > >pipe_bpp);
>
> I would use MISSING_CASE(crtc_state->pipe_bpp); here, as it's pretty
> fatal to VSC setup. :)
>
> Otherwise series looks good from an atomic point of view. I would try
> to get an ACK from the analogix bridge maintainer for
> inclusion in the drm-intel-next-queued tree, since it seems harmless
> enough to include from there.
>
Hi,
Thank you for reviewing the series.
And I'll replace DRM_DEBUG_KMS() to MISSING_CASE() on default switch-
case.
> So for whole series:
> Reviewed-by: Maarten Lankhorst
>
> > + break;
> > + }
> > +
> > +
Re: [PATCH v6 5/6] drm/i915/dp: Change a link bandwidth computation for DP
On Wed, 2019-05-08 at 20:58 +0300, Ville Syrjälä wrote:
> On Wed, May 08, 2019 at 11:17:56AM +0300, Gwan-gyeong Mun wrote:
> > Data M/N calculations were assumed a bpp as RGB format. But when we
> > are
> > using YCbCr 4:2:0 output format on DP, we should change bpp
> > calculations
> > as YCbCr 4:2:0 format. The pipe_bpp value was assumed RGB format,
> > therefore, it was multiplied with 3. But YCbCr 4:2:0 requires a
> > multiplier
> > value to 1.5.
> > Therefore we need to divide pipe_bpp to 2 while DP output uses
> > YCbCr4:2:0
> > format.
> > - RGB format bpp = bpc x 3
> > - YCbCr 4:2:0 format bpp = bpc x 1.5
> >
> > But Link M/N values are calculated and applied based on the Full
> > Clock for
> > YCbCr 4:2:0. And DP YCbCr 4:2:0 does not need to pixel clock double
> > for
> > a dotclock caluation. Only for HDMI YCbCr 4:2:0 needs to pixel
> > clock double
> > for a dot clock calculation.
> >
> > And it adds missed bpc values for a programming of VSC Header.
> > It only affects dp and edp port which use YCbCr 4:2:0 output
> > format.
> > And for now, it does not consider a use case of DSC + YCbCr 4:2:0.
> >
> > v2:
> > Addressed review comments from Ville.
> > Remove a changing of pipe_bpp on intel_ddi_set_pipe_settings().
> > Because the pipe is running at the full bpp, keep pipe_bpp as RGB
> > even though YCbCr 4:2:0 output format is used.
> > Add a link bandwidth computation for YCbCr4:2:0 output format.
> >
> > v3:
> > Addressed reivew comments from Ville.
> > In order to make codes simple, it adds and uses
> > intel_dp_output_bpp()
> > function.
> >
> > v6:
> > Link M/N values are calculated and applied based on the Full
> > Clock for
> > YCbCr420. The Bit per Pixel needs to be adjusted for YUV420 mode
> > as it
> > requires only half of the RGB case.
> > - Link M/N values are calculated and applied based on the Full
> > Clock
> > - Data M/N values needs to be calculated considering the data
> > is half
> > due to subsampling
> > Remove a doubling of pixel clock on a dot clock calculator for
> > DP YCbCr 4:2:0.
> > Rebase and remove a duplicate setting of vsc_sdp.DB17.
> > Add a setting of dynamic range bit to vsc_sdp.DB17.
> > Change Content Type bit to "Graphics" from "Not defined".
> > Change a dividing of pipe_bpp to muliplying to constant values on
> > a
> > switch-case statement.
> >
> > Cc: Ville Syrjälä
> > Signed-off-by: Gwan-gyeong Mun
> > ---
> > drivers/gpu/drm/i915/intel_ddi.c | 3 ++-
> > drivers/gpu/drm/i915/intel_dp.c | 42
> > +---
> > 2 files changed, 41 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_ddi.c
> > b/drivers/gpu/drm/i915/intel_ddi.c
> > index 4441c5ba71fb..e22a0898b957 100644
> > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > @@ -1457,7 +1457,8 @@ static void ddi_dotclock_get(struct
> > intel_crtc_state *pipe_config)
> > else
> > dotclock = pipe_config->port_clock;
> >
> > - if (pipe_config->output_format == INTEL_OUTPUT_FORMAT_YCBCR420)
> > + if (pipe_config->output_format == INTEL_OUTPUT_FORMAT_YCBCR420
> > &&
> > + !intel_crtc_has_dp_encoder(pipe_config))
> > dotclock *= 2;
> >
> > if (pipe_config->pixel_multiplier)
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > b/drivers/gpu/drm/i915/intel_dp.c
> > index 74aad8830a80..c75e2bbe612a 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -1842,6 +1842,19 @@ intel_dp_adjust_compliance_config(struct
> > intel_dp *intel_dp,
> > }
> > }
> >
> > +static int intel_dp_output_bpp(const struct intel_crtc_state
> > *crtc_state, int bpp)
> > +{
> > + /*
> > +* bpp value was assumed to RGB format. And YCbCr 4:2:0 output
> > +* format of the number of bytes per pixel will be half the
> > number
> > +* of bytes of RGB pixel.
> > +*/
> > + if (crtc_state->output_format == INTEL_OUTPUT_FORMAT_YCBCR420)
> > + bpp /= 2;
> > +
> > + return bpp;
> > +}
> > +
> > /* Optimize link config in order: max bpp, min clock, min lanes */
> > static int
> > intel_dp_compute_link_config_wide(struct intel_dp *intel_dp,
> > @@ -2212,7 +2225,7 @@ intel_dp_compute_config(struct intel_encoder
> > *encoder,
> > if (pipe_config->dsc_params.compression_enable)
> > output_bpp = pipe_config->dsc_params.compressed_bpp;
> > else
> > - output_bpp = pipe_config->pipe_bpp;
> > + output_bpp = intel_dp_output_bpp(pipe_config,
> > pipe_config->pipe_bpp);
> >
> > intel_link_compute_m_n(output_bpp,
> >pipe_config->lane_count,
> > @@ -4439,7 +4452,30 @@ intel_pixel_encoding_setup_vsc(struct
> > intel_dp *intel_dp,
> > * 011b = 12bpc.
> > * 100b = 16bpc.
> > */
> > - vsc_sdp.DB17 = 0x1;
> > + switch (crtc_state->pipe_bpp) {
> > + case 24: /* 8bpc */
> > + vsc_sdp.DB17 =
Re: [PATCH v6 3/6] drm/i915/dp: Program VSC Header and DB for Pixel Encoding/Colorimetry Format
On Wed, 2019-05-08 at 20:56 +0300, Ville Syrjälä wrote:
> On Wed, May 08, 2019 at 11:17:54AM +0300, Gwan-gyeong Mun wrote:
> > Function intel_pixel_encoding_setup_vsc handles vsc header and data
> > block
> > setup for pixel encoding / colorimetry format.
> >
> > Setup VSC header and data block in function
> > intel_pixel_encoding_setup_vsc
> > for pixel encoding / colorimetry format as per dp 1.4a spec,
> > section 2.2.5.7.1, table 2-119: VSC SDP Header Bytes, section
> > 2.2.5.7.5,
> > table 2-120:VSC SDP Payload for DB16 through DB18.
> >
> > v2:
> > Minor style fix. [Maarten]
> > Refer to commit ids instead of patchwork. [Maarten]
> >
> > v6: Rebase
> >
> > Cc: Maarten Lankhorst
> > Signed-off-by: Gwan-gyeong Mun
> > ---
> > drivers/gpu/drm/i915/intel_ddi.c | 1 +
> > drivers/gpu/drm/i915/intel_dp.c | 73
> >
> > drivers/gpu/drm/i915/intel_drv.h | 2 +
> > 3 files changed, 76 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_ddi.c
> > b/drivers/gpu/drm/i915/intel_ddi.c
> > index cd5277d98b03..2f1688ea5a2c 100644
> > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > @@ -3391,6 +3391,7 @@ static void intel_enable_ddi_dp(struct
> > intel_encoder *encoder,
> >
> > intel_edp_backlight_on(crtc_state, conn_state);
> > intel_psr_enable(intel_dp, crtc_state);
> > + intel_dp_ycbcr_420_enable(intel_dp, crtc_state);
>
> I wonder if this is a bit too late. But we do it for PSR here, so I
> guess we should think about this for both cases. We should actually
> add full readout + state checker for the VSC SDP for both cases as
> well. But that can be done later.
>
I'll check it again.
> > intel_edp_drrs_enable(intel_dp, crtc_state);
> >
> > if (crtc_state->has_audio)
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > b/drivers/gpu/drm/i915/intel_dp.c
> > index 06a3417a88d1..74aad8830a80 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -4394,6 +4394,79 @@ u8 intel_dp_dsc_get_slice_count(struct
> > intel_dp *intel_dp,
> > return 0;
> > }
> >
> > +static void
> > +intel_pixel_encoding_setup_vsc(struct intel_dp *intel_dp,
> > + const struct intel_crtc_state
> > *crtc_state)
> > +{
> > + struct intel_digital_port *intel_dig_port =
> > dp_to_dig_port(intel_dp);
> > + struct dp_vsc_sdp vsc_sdp;
> > +
> > + if (!intel_dp->attached_connector->base.ycbcr_420_allowed)
> > + return;
> > +
> > + /* Prepare VSC Header for SU as per DP 1.4a spec, Table 2-119
> > */
> > + memset(_sdp, 0, sizeof(vsc_sdp));
>
> Can be replaced with '= {}' in the declaration.
>
Yes, I'll use a structure initializer instead of memset().
> > + vsc_sdp.sdp_header.HB0 = 0;
> > + vsc_sdp.sdp_header.HB1 = 0x7;
> > +
> > + /* VSC SDP supporting 3D stereo, PSR2, and Pixel Encoding/
> > +* Colorimetry Format indication. A DP Source device is allowed
> > +* to indicate the pixel encoding/colorimetry format to the DP
> > Sink
> > +* device with VSC SDP only when the DP Sink device supports it
> > +* (i.e., VSC_SDP_EXTENSION_FOR_COLORIMETRY_SUPPORTED bit in
> > the register
> > +* DPRX_FEATURE_ENUMERATION_LIST (DPCD Address 02210h, bit 3)
> > is set to 1)
> > +*/
>
> Are we checking that bit somewhere? I suppose the sink might a bit
> nuts
> if it declares 420_only modes without that set, but maybe it should
> be
> checked anyway.
>
> Also non-standard comment format all over. Should be
> /*
> * blah
> */
>
I'll add a checking of VSC_SDP_EXTENSION_FOR_COLORIMETRY_SUPPORTED bit
with intel_dp_get_colorimetry_status() on intel_dp_ycbcr420_config().
And I will fix non-standard comment format.
> > + vsc_sdp.sdp_header.HB2 = 0x5;
> > +
> > + /* VSC SDP supporting 3D stereo, + PSR2, + Pixel Encoding/
> > +* Colorimetry Format indication (HB2 = 05h).
> > +*/
> > + vsc_sdp.sdp_header.HB3 = 0x13;
> > + /* YCbCr 420 = 3h DB16[7:4] ITU-R BT.601 = 0h, ITU-R BT.709 =
> > 1h
> > +* DB16[3:0] DP 1.4a spec, Table 2-120
> > +*/
> > +
> > + /* Commit id (25edf91501b8 "drm/i915: prepare csc unit for
> > YCBCR420 output")
> > +* uses the BT.709 color space to perform RGB->YCBCR
> > conversion.
> > +*/
>
> I don't think referring to specific commit here is particularly
> helpful.
> The situation will change anyway at some point.
>
Yes, I'll remove a referring to specific commit.
> > + vsc_sdp.DB16 = 0x3 << 4; /* 0x3 << 4 , YCbCr 420*/
> > + vsc_sdp.DB16 |= 0x1; /* 0x1, ITU-R BT.709 */
> > +
> > + /* For pixel encoding formats YCbCr444, YCbCr422, YCbCr420, and
> > Y Only,
> > +* the following Component Bit Depth values are defined:
> > +* 001b = 8bpc.
> > +* 010b = 10bpc.
> > +* 011b = 12bpc.
> > +* 100b = 16bpc.
> > +*/
> > + vsc_sdp.DB17 = 0x1;
>
> Don't we need to base this on pipe_bpp?
>
> And I'm thinking we want the CEA bit set here as well.
>
I'll move a
Re: [PATCH v6 2/6] drm: Add a VSC structure for handling Pixel Encoding/Colorimetry Formats
On Wed, 2019-05-08 at 20:32 +0300, Ville Syrjälä wrote:
> On Wed, May 08, 2019 at 11:17:53AM +0300, Gwan-gyeong Mun wrote:
> > SDP VSC Header and Data Block follow DP 1.4a spec, section
> > 2.2.5.7.5,
> > chapter "VSC SDP Payload for Pixel Encoding/Colorimetry Format".
> >
> > Signed-off-by: Gwan-gyeong Mun
> > Reviewed-by: Maarten Lankhorst
> > ---
> > include/drm/drm_dp_helper.h | 17 +
> > 1 file changed, 17 insertions(+)
> >
> > diff --git a/include/drm/drm_dp_helper.h
> > b/include/drm/drm_dp_helper.h
> > index 97ce790a5b5a..3793bea7b7fe 100644
> > --- a/include/drm/drm_dp_helper.h
> > +++ b/include/drm/drm_dp_helper.h
> > @@ -1096,6 +1096,23 @@ struct edp_vsc_psr {
> > u8 DB8_31[24]; /* Reserved */
> > } __packed;
> >
> > +struct dp_vsc_sdp {
> > + struct dp_sdp_header sdp_header;
> > + u8 DB0; /* Stereo Interface */
> > + u8 DB1; /* 0 - PSR State; 1 - Update RFB; 2 - CRC Valid */
> > + u8 DB2; /* CRC value bits 7:0 of the R or Cr component */
> > + u8 DB3; /* CRC value bits 15:8 of the R or Cr component */
> > + u8 DB4; /* CRC value bits 7:0 of the G or Y component */
> > + u8 DB5; /* CRC value bits 15:8 of the G or Y component */
> > + u8 DB6; /* CRC value bits 7:0 of the B or Cb component */
> > + u8 DB7; /* CRC value bits 15:8 of the B or Cb component */
> > + u8 DB8_15[8]; /* Reserved */
> > + u8 DB16; /* Pixel Encoding and Colorimetry Formats */
> > + u8 DB17; /* Dynamic Range and Component Bit Depth */
> > + u8 DB18; /* Content Type */
> > + u8 DB19_31[13]; /* Reserved */
> > +} __packed;
>
> Isn't this the same thing we have for edp already? Just rename the
> edp
> struct and add the missing stuff?
>
Okay, I'll rename struct edp_vsc_psr to general name of dp_sdp and will
add missing stuff.
> > +
> > #define EDP_VSC_PSR_STATE_ACTIVE (1<<0)
> > #define EDP_VSC_PSR_UPDATE_RFB (1<<1)
> > #define EDP_VSC_PSR_CRC_VALUES_VALID (1<<2)
> > --
> > 2.21.0
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [Intel-gfx] [PATCH v2] drm: Fire off KMS hotplug events if probe detect says the connector is connected
On Thu, Apr 18, 2019 at 7:33 PM Jani Nikula wrote: > > On Thu, 18 Apr 2019, Gwan-gyeong Mun wrote: > > The hotplug detection routine of drm_helper_hpd_irq_event() can detect > > changing of status of connector, but it can not detect changing of > > properties of the connector. > > e.g. changing of edid while suspend/resume, changing of dp lanes in dp aux. > > > > Following scenario explains one of them; A detection of changing of edid. > > > > 1) plug display device to a connector > > 2) system suspend > > 3) unplug 1)'s display device and plug the other display device to a > > connector > > 4) system resume > > > > To solve explained cases, It fires off KMS hotplug events if > > drm_helper_probe_detect() says the connector is connected. > > > > Testcase: igt/kms_chamelium/hdmi-edid-change-during-hibernate > > Testcase: igt/kms_chamelium/hdmi-edid-change-during-suspend > > Testcase: igt/kms_chamelium/dp-edid-change-during-hibernate > > Testcase: igt/kms_chamelium/dp-edid-change-during-suspend > > > > v2: Remove suggested-by line (danvet) > > Signed-off-by: Gwan-gyeong Mun > > Link: > > https://lists.freedesktop.org/archives/dri-devel/2019-April/214572.html > > Link: is something added by our tooling (dim), not to be added > yourself. It points at the patch at patchwork using the message-id of > the patch email. > > If you want to refer to some other work, please use References: tag. > > BR, > Jani. > > Hi, Sorry for misusing of that. I didn't know that. I'll replace Link to References and resend the patch as soon as possible. Thank you for letting me know that. Br, G.G. > > --- > > drivers/gpu/drm/drm_probe_helper.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/drm_probe_helper.c > > b/drivers/gpu/drm/drm_probe_helper.c > > index 6fd08e04b323..081a849104f2 100644 > > --- a/drivers/gpu/drm/drm_probe_helper.c > > +++ b/drivers/gpu/drm/drm_probe_helper.c > > @@ -780,7 +780,8 @@ bool drm_helper_hpd_irq_event(struct drm_device *dev) > > connector->name, > > drm_get_connector_status_name(old_status), > > > > drm_get_connector_status_name(connector->status)); > > - if (old_status != connector->status) > > + if (old_status != connector->status || > > + connector->status == connector_status_connected) > > changed = true; > > } > > drm_connector_list_iter_end(_iter); > > -- > Jani Nikula, Intel Open Source Graphics Center > ___ > Intel-gfx mailing list > intel-...@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Gwan-gyeong Mun ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm: Fire off KMS hotplug events if probe detect says the connector is connected
On Thu, 2019-04-18 at 10:28 +0200, Daniel Vetter wrote: > On Thu, Apr 18, 2019 at 11:09:29AM +0300, Gwan-gyeong Mun wrote: > > The hotplug detection routine of drm_helper_hpd_irq_event() can > > detect > > changing of status of connector, but it can not detect changing of > > properties of the connector. > > e.g. changing of edid while suspend/resume, changing of dp lanes in > > dp aux. > > > > Following scenario explains one of them; A detection of changing of > > edid. > > > > 1) plug display device to a connector > > 2) system suspend > > 3) unplug 1)'s display device and plug the other display device to > > a > > connector > > 4) system resume > > > > To solve explained cases, It fires off KMS hotplug events if > > drm_helper_probe_detect() says the connector is connected. > > > > Testcase: igt/kms_chamelium/hdmi-edid-change-during-hibernate > > Testcase: igt/kms_chamelium/hdmi-edid-change-during-suspend > > Testcase: igt/kms_chamelium/dp-edid-change-during-hibernate > > Testcase: igt/kms_chamelium/dp-edid-change-during-suspend > > > > Suggested-by: Daniel Vetter > > This isn't at all what I suggested. > -Daniel Because the code modification was followed by your comments, so I added suggested-by line. I will remove this line and will resend it. Br, G.G. > > > Signed-off-by: Gwan-gyeong Mun > > Link: > > https://lists.freedesktop.org/archives/dri-devel/2019-April/214572.html > > --- > > drivers/gpu/drm/drm_probe_helper.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/drm_probe_helper.c > > b/drivers/gpu/drm/drm_probe_helper.c > > index 6fd08e04b323..081a849104f2 100644 > > --- a/drivers/gpu/drm/drm_probe_helper.c > > +++ b/drivers/gpu/drm/drm_probe_helper.c > > @@ -780,7 +780,8 @@ bool drm_helper_hpd_irq_event(struct drm_device > > *dev) > > connector->name, > > drm_get_connector_status_name(old_status) > > , > > drm_get_connector_status_name(connector- > > >status)); > > - if (old_status != connector->status) > > + if (old_status != connector->status || > > + connector->status == connector_status_connected) > > changed = true; > > } > > drm_connector_list_iter_end(_iter); > > -- > > 2.21.0 > > ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [Intel-gfx] [PATCH v5 0/2] drm: Add detection of changing of edid on between suspend and resume
On Mon, 2019-04-15 at 18:32 +0200, Daniel Vetter wrote: > On Thu, Apr 11, 2019 at 05:36:30PM +0300, Gwan-gyeong Mun wrote: > > This patch series fix missed detection of changing of edid on > > between > > suspend and resume. > > First patch fixes drm_helper_hdp_irq_event() in order to fix a > > below use > > case. > > > > Following scenario requires detection of changing of edid. > > > > 1) plug display device to a connector > > 2) system suspend > > 3) unplug 1)'s display device and plug the other display device > > to a > > connector > > 4) system resume > > > > It adds edid check routine when a connector status still remains as > > "connector_status_connected". > > > > Second patch adds a missed update of edid property of drm connector > > on i915. > > > > v2: Add NULL check before comparing of EDIDs. > > v3: Make it as part of existing drm_helper_hpd_irq_event() (Stan, > > Mika) > > v4: Rebased > > v5: Use a cached edid property blob data of connector instead of > > adding > > a new detected_edid variable. (Maarten) > > Add an using of reference count for getting a cached edid > > property > > blob data. (Maarten) > > > > Testcase: igt/kms_chamelium/hdmi-edid-change-during-hibernate > > Testcase: igt/kms_chamelium/hdmi-edid-change-during-suspend > > Testcase: igt/kms_chamelium/dp-edid-change-during-hibernate > > Testcase: igt/kms_chamelium/dp-edid-change-during-suspend > > > > v1, v2: https://patchwork.freedesktop.org/series/47680/ > > v3: https://patchwork.freedesktop.org/series/49298/ > > v4: https://patchwork.freedesktop.org/series/57397/ > > I guess I missed this, but there's a few fundamental things here > first > imo: > > - patch 2 is fallout from the fairly abitrary split between ->detect > and > ->get_modes. There's not really any good reason for that, I think > we can > just always call ->get_modes if ->detect says the connector is > connected. There's more than just dp and hdmi (that's simply the 2 > things we test in CI using chamelium). > Thank you for checking it. I agree. I'll modify first patch with your guide and will resend it. > - the problem is bigger than just the edid changing (e.g. when > repeaters > change, or when something else changes aside from the edid, e.g. in > dp > aux, like how many lanes the dp cable has). Plus we have this long- > term > idea to give userspace a better idea about which connector exactly > has > changed. Therefor: > Yes, you pointed an essential problem. > 1. add a new drm_connector->detect_epoque counter. > 2. Increment that counter every time something has changed, i.e. > from > probe helpers (we can push this down into the detect wrappers) when > connector->status has changed, or when we update the edid blob. > 3. probe helpers look for changes in ->detect_epoque to decided > whether > to fire the uevent or not. > 4. long term we could expose the ->detect_epoque as a read-only > property, or at least supply information about which connector > caused > the uevent using the new drm_sysfs_connector_status_event() (see > the > patch from Ram "[PATCH v3 09/10] drm: uevent for connector status > change"). > I'll make new patches which followes your guide. Thanks for reviewing it and giving me a new approach. Br, G.G. > I think that gives us a much better long term foundation than adding > lots > of hacks. > > Cheers, Daniel > > > Gwan-gyeong Mun (2): > > drm: Add detection of changing of edid on between suspend and > > resume > > drm/i915: Add a missed update of edid property of drm connector > > > > drivers/gpu/drm/drm_probe_helper.c | 34 > > +- > > drivers/gpu/drm/i915/intel_dp.c| 1 + > > drivers/gpu/drm/i915/intel_hdmi.c | 1 + > > 3 files changed, 35 insertions(+), 1 deletion(-) > > > > -- > > 2.21.0 > > > > ___ > > Intel-gfx mailing list > > intel-...@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v5 1/2] drm: Add detection of changing of edid on between suspend and resume
On Thu, 2019-04-11 at 17:00 +0100, Lisovskiy, Stanislav wrote:
> On Thu, 2019-04-11 at 17:36 +0300, Gwan-gyeong Mun wrote:
> > The hotplug detection routine of drm_helper_hpd_irq_event() can
> > detect
> > changing of status of connector, but it can not detect changing of
> > edid.
> >
> > Following scenario requires detection of changing of edid.
> >
> > 1) plug display device to a connector
> > 2) system suspend
> > 3) unplug 1)'s display device and plug the other display device to
> > a
> > connector
> > 4) system resume
> >
> > It adds edid check routine when a connector status still remains as
> > "connector_status_connected".
> >
> > v2: Add NULL check before comparing of EDIDs.
> > v3: Make it as part of existing drm_helper_hpd_irq_event() (Stan,
> > Mika)
> > v4: Rebased
> > v5: Use a cached edid property blob data of connector instead of
> > adding
> > a new detected_edid variable. (Maarten)
> > Add an using of reference count for getting a cached edid
> > property
> > blob data. (Maarten)
> >
> > Testcase: igt/kms_chamelium/hdmi-edid-change-during-hibernate
> > Testcase: igt/kms_chamelium/hdmi-edid-change-during-suspend
> > Testcase: igt/kms_chamelium/dp-edid-change-during-hibernate
> > Testcase: igt/kms_chamelium/dp-edid-change-during-suspend
> >
> > Signed-off-by: Gwan-gyeong Mun
> > ---
> > drivers/gpu/drm/drm_probe_helper.c | 34
> > +-
> > 1 file changed, 33 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/drm_probe_helper.c
> > b/drivers/gpu/drm/drm_probe_helper.c
> > index 6fd08e04b323..27ad7f3dabb7 100644
> > --- a/drivers/gpu/drm/drm_probe_helper.c
> > +++ b/drivers/gpu/drm/drm_probe_helper.c
> > @@ -742,7 +742,16 @@ EXPORT_SYMBOL(drm_kms_helper_poll_fini);
> > * panels.
> > *
> > * This helper function is useful for drivers which can't or don't
> > track hotplug
> > - * interrupts for each connector.
> > + * interrupts for each connector. And it also supports a detection
> > of changing
> > + * of edid on between suspend and resume when a connector status
> > still remains
> > + * as "connector_status_connected".
> > + *
> > + * Following scenario requires detection of changing of edid.
> > + * 1) plug display device to a connector
> > + * 2) system suspend
> > + * 3) unplug 1)'s display device and plug the other display
> > device
> > to a
> > + * connector
> > + * 4) system resume
> > *
> > * Drivers which support hotplug interrupts for each connector
> > individually and
> > * which have a more fine-grained detect logic should bypass this
> > code and
> > @@ -760,6 +769,7 @@ bool drm_helper_hpd_irq_event(struct drm_device
> > *dev)
> > struct drm_connector *connector;
> > struct drm_connector_list_iter conn_iter;
> > enum drm_connector_status old_status;
> > + struct drm_property_blob *old_edid_blob_ptr;
> > bool changed = false;
> >
> > if (!dev->mode_config.poll_enabled)
> > @@ -774,6 +784,11 @@ bool drm_helper_hpd_irq_event(struct
> > drm_device
> > *dev)
> >
> > old_status = connector->status;
> >
> > + if (connector->edid_blob_ptr)
> > + old_edid_blob_ptr =
> > drm_property_blob_get(connector->edid_blob_ptr);
> > + else
> > + old_edid_blob_ptr = NULL;
> > +
> > connector->status = drm_helper_probe_detect(connector,
> > NULL, false);
> > DRM_DEBUG_KMS("[CONNECTOR:%d:%s] status updated from %s
> > to %s\n",
> > connector->base.id,
> > @@ -782,6 +797,23 @@ bool drm_helper_hpd_irq_event(struct
> > drm_device
> > *dev)
> > drm_get_connector_status_name(connector-
> > > status));
> > if (old_status != connector->status)
> > changed = true;
> > +
> > + /* Check changing of edid when a connector status still
> > remains
> > +* as "connector_status_connected".
> > +*/
> > + if (old_edid_blob_ptr && connector->edid_blob_ptr &&
>
> I guess you don't need to check both old_edid_blob_ptr && connector-
> > edid_blob_ptr here. Because if old_edid_blob_ptr is not NULL - it
> means that connector->edid_blob_ptr was not NULL for sure. See the
> condition you have added above.
> I mean this one:
>
> > + if (connector->edid_blob_ptr)
> > + old_edid_blob_ptr =
> > drm_property_blob_get(connector->edid_blob_ptr);
> > + else
> > + old_edid_blob_ptr = NULL;
>
> So I would check only old_edid_blob_ptr for not being NULL here.
>
>
Hi Stan,
Thank you for checking it.
First, drivers could set edid_blob_ptr to NULL with
drm_connector_update_edid_property().
And drm_helper_probe_detect() could lead updating of an internal edid
of driver and if driver can not get EDID from I2C, it could call
drm_connector_update_edid_property() with NULL.
therefore I think that we should check old and new edid_blob_ptr are
Re: [Intel-gfx] [PATCH] drm/i915: Fix typo in i915_drm_resume()
On Fri, 2018-08-03 at 20:12 +0100, Chris Wilson wrote: > Quoting Gwan-gyeong Mun (2018-08-03 17:41:50) > > Even for trivial patches, always include a changelog. In this case, I > added "Trivial typo, s/loose/lose/, in i915_drm_resume." > > > Signed-off-by: Gwan-gyeong Mun > > Reviewed-by: Chris Wilson > > And pushed, thanks for the spelling correction. > -Chris Hi Chris, Thank you for guiding me and reviewing it. - Gwan-gyeong. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
