Re: [Intel-gfx] [PATCH 13/18] drm/i915: Precompute HDMI infoframes
On Mon, Sep 24, 2018 at 05:58:25PM +0200, Daniel Vetter wrote: > On Thu, Sep 20, 2018 at 09:51:40PM +0300, Ville Syrjala wrote: > > From: Ville Syrjälä > > > > Store the infoframes in the crtc state and precompute them in > > .compute_config(). While precomputing we'll also fill out the > > inforames.enable bitmask appropriately. > > > > Signed-off-by: Ville Syrjälä > > --- > > drivers/gpu/drm/i915/intel_ddi.c | 1 + > > drivers/gpu/drm/i915/intel_drv.h | 5 + > > drivers/gpu/drm/i915/intel_hdmi.c | 249 > > +++--- > > 3 files changed, 187 insertions(+), 68 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c > > b/drivers/gpu/drm/i915/intel_ddi.c > > index 19fef88e680e..5f3bd536d261 100644 > > --- a/drivers/gpu/drm/i915/intel_ddi.c > > +++ b/drivers/gpu/drm/i915/intel_ddi.c > > @@ -3391,6 +3391,7 @@ void intel_ddi_get_config(struct intel_encoder > > *encoder, > > intel_dig_port = enc_to_dig_port(>base); > > > > pipe_config->infoframes.enable |= > > + intel_hdmi_infoframe_enable(HDMI_PACKET_TYPE_NULL) | > > Misplaced hunk? Assuming I'm reading this correctly, this will give you a > 0. Not exactly sure what's going on here ... I guess I'm not following why > we care about TYPE_NULL, and why we have to reconstruct it here? I rather wanted the infoframes bitmask to be truthful about which packets we're sending out. But not quite sure why I included it in this particular patch. > > I thought TYPE_NULL is equivalent to state->has_hdmi_sink? Comment > (unfortunately not yet kerneldoc) even explains that ... Yeah it's the same thing (apart from the g4x DIP enable thing). Maybe I should remove has_hdmi_sink entirely and just rely on the null packet bit instead... > > Maybe just nuke all the TYPE_NULL tracking here, perhaps with the g4x > (except for g4x itself, because it's shared there) decoder to also take > DIP_ENABLE into account for has_hdmi_sink. That would the other option I suppose. Though I might like the idea of dropping the bool for the bitmask a bit more perhaps. > > Or update the comment in intel_crtc_state. > > Otherwise lgtm, thought admittedly I did clean over the details a bit, > trusting CI and gcc to catch the small stuff :-) > > Reviewed-by: Daniel Vetter with the TYPE_NULL > story somehow figured out. > -Daniel > > > > intel_hdmi_infoframes_enabled(encoder, pipe_config); > > > > if (pipe_config->infoframes.enable) > > diff --git a/drivers/gpu/drm/i915/intel_drv.h > > b/drivers/gpu/drm/i915/intel_drv.h > > index 50c0c049ee15..357624a6bfe2 100644 > > --- a/drivers/gpu/drm/i915/intel_drv.h > > +++ b/drivers/gpu/drm/i915/intel_drv.h > > @@ -895,6 +895,10 @@ struct intel_crtc_state { > > > > struct { > > u32 enable; > > + u32 gcp; > > + union hdmi_infoframe avi; > > + union hdmi_infoframe spd; > > + union hdmi_infoframe hdmi; > > } infoframes; > > > > /* HDMI scrambling status */ > > @@ -1862,6 +1866,7 @@ void intel_dp_dual_mode_set_tmds_output(struct > > intel_hdmi *hdmi, bool enable); > > void intel_infoframe_init(struct intel_digital_port *intel_dig_port); > > u32 intel_hdmi_infoframes_enabled(struct intel_encoder *encoder, > > const struct intel_crtc_state *crtc_state); > > +u32 intel_hdmi_infoframe_enable(unsigned int type); > > > > > > /* intel_lvds.c */ > > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c > > b/drivers/gpu/drm/i915/intel_hdmi.c > > index 98a44084324c..491001fc0fad 100644 > > --- a/drivers/gpu/drm/i915/intel_hdmi.c > > +++ b/drivers/gpu/drm/i915/intel_hdmi.c > > @@ -446,6 +446,18 @@ static const u8 infoframe_type_to_idx[] = { > > HDMI_INFOFRAME_TYPE_VENDOR, > > }; > > > > +u32 intel_hdmi_infoframe_enable(unsigned int type) > > +{ > > + int i; > > + > > + for (i = 0; i < ARRAY_SIZE(infoframe_type_to_idx); i++) { > > + if (infoframe_type_to_idx[i] == type) > > + return BIT(i); > > + } > > + > > + return 0; > > +} > > + > > u32 intel_hdmi_infoframes_enabled(struct intel_encoder *encoder, > > const struct intel_crtc_state *crtc_state) > > { > > @@ -491,15 +503,23 @@ u32 intel_hdmi_infoframes_enabled(struct > > intel_encoder *encoder, > > */ > > static void intel_write_infoframe(struct intel_encoder *encoder, > > const struct intel_crtc_state *crtc_state, > > - union hdmi_infoframe *frame) > > + enum hdmi_infoframe_type type, > > + const union hdmi_infoframe *frame) > > { > > struct intel_digital_port *intel_dig_port = > > enc_to_dig_port(>base); > > u8 buffer[VIDEO_DIP_DATA_SIZE]; > > ssize_t len; > > > > + if ((crtc_state->infoframes.enable & > > +intel_hdmi_infoframe_enable(type)) == 0) > > +
Re: [Intel-gfx] [PATCH 13/18] drm/i915: Precompute HDMI infoframes
On Thu, Sep 20, 2018 at 09:51:40PM +0300, Ville Syrjala wrote: > From: Ville Syrjälä > > Store the infoframes in the crtc state and precompute them in > .compute_config(). While precomputing we'll also fill out the > inforames.enable bitmask appropriately. > > Signed-off-by: Ville Syrjälä > --- > drivers/gpu/drm/i915/intel_ddi.c | 1 + > drivers/gpu/drm/i915/intel_drv.h | 5 + > drivers/gpu/drm/i915/intel_hdmi.c | 249 > +++--- > 3 files changed, 187 insertions(+), 68 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c > b/drivers/gpu/drm/i915/intel_ddi.c > index 19fef88e680e..5f3bd536d261 100644 > --- a/drivers/gpu/drm/i915/intel_ddi.c > +++ b/drivers/gpu/drm/i915/intel_ddi.c > @@ -3391,6 +3391,7 @@ void intel_ddi_get_config(struct intel_encoder *encoder, > intel_dig_port = enc_to_dig_port(>base); > > pipe_config->infoframes.enable |= > + intel_hdmi_infoframe_enable(HDMI_PACKET_TYPE_NULL) | Misplaced hunk? Assuming I'm reading this correctly, this will give you a 0. Not exactly sure what's going on here ... I guess I'm not following why we care about TYPE_NULL, and why we have to reconstruct it here? I thought TYPE_NULL is equivalent to state->has_hdmi_sink? Comment (unfortunately not yet kerneldoc) even explains that ... Maybe just nuke all the TYPE_NULL tracking here, perhaps with the g4x (except for g4x itself, because it's shared there) decoder to also take DIP_ENABLE into account for has_hdmi_sink. Or update the comment in intel_crtc_state. Otherwise lgtm, thought admittedly I did clean over the details a bit, trusting CI and gcc to catch the small stuff :-) Reviewed-by: Daniel Vetter with the TYPE_NULL story somehow figured out. -Daniel > intel_hdmi_infoframes_enabled(encoder, pipe_config); > > if (pipe_config->infoframes.enable) > diff --git a/drivers/gpu/drm/i915/intel_drv.h > b/drivers/gpu/drm/i915/intel_drv.h > index 50c0c049ee15..357624a6bfe2 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -895,6 +895,10 @@ struct intel_crtc_state { > > struct { > u32 enable; > + u32 gcp; > + union hdmi_infoframe avi; > + union hdmi_infoframe spd; > + union hdmi_infoframe hdmi; > } infoframes; > > /* HDMI scrambling status */ > @@ -1862,6 +1866,7 @@ void intel_dp_dual_mode_set_tmds_output(struct > intel_hdmi *hdmi, bool enable); > void intel_infoframe_init(struct intel_digital_port *intel_dig_port); > u32 intel_hdmi_infoframes_enabled(struct intel_encoder *encoder, > const struct intel_crtc_state *crtc_state); > +u32 intel_hdmi_infoframe_enable(unsigned int type); > > > /* intel_lvds.c */ > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c > b/drivers/gpu/drm/i915/intel_hdmi.c > index 98a44084324c..491001fc0fad 100644 > --- a/drivers/gpu/drm/i915/intel_hdmi.c > +++ b/drivers/gpu/drm/i915/intel_hdmi.c > @@ -446,6 +446,18 @@ static const u8 infoframe_type_to_idx[] = { > HDMI_INFOFRAME_TYPE_VENDOR, > }; > > +u32 intel_hdmi_infoframe_enable(unsigned int type) > +{ > + int i; > + > + for (i = 0; i < ARRAY_SIZE(infoframe_type_to_idx); i++) { > + if (infoframe_type_to_idx[i] == type) > + return BIT(i); > + } > + > + return 0; > +} > + > u32 intel_hdmi_infoframes_enabled(struct intel_encoder *encoder, > const struct intel_crtc_state *crtc_state) > { > @@ -491,15 +503,23 @@ u32 intel_hdmi_infoframes_enabled(struct intel_encoder > *encoder, > */ > static void intel_write_infoframe(struct intel_encoder *encoder, > const struct intel_crtc_state *crtc_state, > - union hdmi_infoframe *frame) > + enum hdmi_infoframe_type type, > + const union hdmi_infoframe *frame) > { > struct intel_digital_port *intel_dig_port = > enc_to_dig_port(>base); > u8 buffer[VIDEO_DIP_DATA_SIZE]; > ssize_t len; > > + if ((crtc_state->infoframes.enable & > + intel_hdmi_infoframe_enable(type)) == 0) > + return; > + > + if (WARN_ON(frame->any.type != type)) > + return; > + > /* see comment above for the reason for this offset */ > - len = hdmi_infoframe_pack(frame, buffer + 1, sizeof(buffer) - 1); > - if (len < 0) > + len = hdmi_infoframe_pack_only(frame, buffer + 1, sizeof(buffer) - 1); > + if (WARN_ON(len < 0)) > return; > > /* Insert the 'hole' (see big comment above) at position 3 */ > @@ -507,85 +527,111 @@ static void intel_write_infoframe(struct intel_encoder > *encoder, > buffer[3] = 0; > len++; > > - intel_dig_port->write_infoframe(encoder, > -
[Intel-gfx] [PATCH 13/18] drm/i915: Precompute HDMI infoframes
From: Ville Syrjälä Store the infoframes in the crtc state and precompute them in .compute_config(). While precomputing we'll also fill out the inforames.enable bitmask appropriately. Signed-off-by: Ville Syrjälä --- drivers/gpu/drm/i915/intel_ddi.c | 1 + drivers/gpu/drm/i915/intel_drv.h | 5 + drivers/gpu/drm/i915/intel_hdmi.c | 249 +++--- 3 files changed, 187 insertions(+), 68 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c index 19fef88e680e..5f3bd536d261 100644 --- a/drivers/gpu/drm/i915/intel_ddi.c +++ b/drivers/gpu/drm/i915/intel_ddi.c @@ -3391,6 +3391,7 @@ void intel_ddi_get_config(struct intel_encoder *encoder, intel_dig_port = enc_to_dig_port(>base); pipe_config->infoframes.enable |= + intel_hdmi_infoframe_enable(HDMI_PACKET_TYPE_NULL) | intel_hdmi_infoframes_enabled(encoder, pipe_config); if (pipe_config->infoframes.enable) diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 50c0c049ee15..357624a6bfe2 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -895,6 +895,10 @@ struct intel_crtc_state { struct { u32 enable; + u32 gcp; + union hdmi_infoframe avi; + union hdmi_infoframe spd; + union hdmi_infoframe hdmi; } infoframes; /* HDMI scrambling status */ @@ -1862,6 +1866,7 @@ void intel_dp_dual_mode_set_tmds_output(struct intel_hdmi *hdmi, bool enable); void intel_infoframe_init(struct intel_digital_port *intel_dig_port); u32 intel_hdmi_infoframes_enabled(struct intel_encoder *encoder, const struct intel_crtc_state *crtc_state); +u32 intel_hdmi_infoframe_enable(unsigned int type); /* intel_lvds.c */ diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c index 98a44084324c..491001fc0fad 100644 --- a/drivers/gpu/drm/i915/intel_hdmi.c +++ b/drivers/gpu/drm/i915/intel_hdmi.c @@ -446,6 +446,18 @@ static const u8 infoframe_type_to_idx[] = { HDMI_INFOFRAME_TYPE_VENDOR, }; +u32 intel_hdmi_infoframe_enable(unsigned int type) +{ + int i; + + for (i = 0; i < ARRAY_SIZE(infoframe_type_to_idx); i++) { + if (infoframe_type_to_idx[i] == type) + return BIT(i); + } + + return 0; +} + u32 intel_hdmi_infoframes_enabled(struct intel_encoder *encoder, const struct intel_crtc_state *crtc_state) { @@ -491,15 +503,23 @@ u32 intel_hdmi_infoframes_enabled(struct intel_encoder *encoder, */ static void intel_write_infoframe(struct intel_encoder *encoder, const struct intel_crtc_state *crtc_state, - union hdmi_infoframe *frame) + enum hdmi_infoframe_type type, + const union hdmi_infoframe *frame) { struct intel_digital_port *intel_dig_port = enc_to_dig_port(>base); u8 buffer[VIDEO_DIP_DATA_SIZE]; ssize_t len; + if ((crtc_state->infoframes.enable & +intel_hdmi_infoframe_enable(type)) == 0) + return; + + if (WARN_ON(frame->any.type != type)) + return; + /* see comment above for the reason for this offset */ - len = hdmi_infoframe_pack(frame, buffer + 1, sizeof(buffer) - 1); - if (len < 0) + len = hdmi_infoframe_pack_only(frame, buffer + 1, sizeof(buffer) - 1); + if (WARN_ON(len < 0)) return; /* Insert the 'hole' (see big comment above) at position 3 */ @@ -507,85 +527,111 @@ static void intel_write_infoframe(struct intel_encoder *encoder, buffer[3] = 0; len++; - intel_dig_port->write_infoframe(encoder, - crtc_state, - frame->any.type, buffer, len); + intel_dig_port->write_infoframe(encoder, crtc_state, type, buffer, len); } -static void intel_hdmi_set_avi_infoframe(struct intel_encoder *encoder, -const struct intel_crtc_state *crtc_state, -const struct drm_connector_state *conn_state) +static bool +intel_hdmi_compute_avi_infoframe(struct intel_encoder *encoder, +struct intel_crtc_state *crtc_state, +struct drm_connector_state *conn_state) { struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(>base); + struct hdmi_avi_infoframe *frame = _state->infoframes.avi.avi; + bool is_hdmi2_sink = conn_state->connector->display_info.hdmi.scdc.supported; const struct drm_display_mode *adjusted_mode = _state->base.adjusted_mode; - struct