> Subject: RE: [PATCH v7 4/8] drm/i915/bios: de/allocate VS/PE-O buffer for > each port > > > <[email protected]>; Grzelak, Michal <[email protected]> > > Subject: [PATCH v7 4/8] drm/i915/bios: de/allocate VS/PE-O buffer for > > each port > > > > Every devdata needs a separate intel_ddi_buf_trans since each port can > > request an override. Add buffer's pointer into intel_bios_encoder_data. > > > > Allocate struct intel_ddi_buf_trans for the port if VS/PE-O was > > requested and is supported. At the same time, allocate struct > > intel_ddi_buf_trans_entry and store it inside struct intel_ddi_buf_trans. > > > > Deallocate the buffer as well as entries if the request is supported. > > > > v4->v5 > > - set devdata->vspeo->num_entries in intel_bios.c > > > > Signed-off-by: Michał Grzelak <[email protected]> > > --- > > drivers/gpu/drm/i915/display/intel_bios.c | 32 > > +++++++++++++++++++++++ > > 1 file changed, 32 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_bios.c > > b/drivers/gpu/drm/i915/display/intel_bios.c > > index 70467344f08e..3d8864374cac 100644 > > --- a/drivers/gpu/drm/i915/display/intel_bios.c > > +++ b/drivers/gpu/drm/i915/display/intel_bios.c > > @@ -34,6 +34,7 @@ > > #include <drm/drm_fixed.h> > > #include <drm/drm_print.h> > > > > +#include "intel_ddi_buf_trans.h" > > #include "intel_display.h" > > #include "intel_display_core.h" > > #include "intel_display_rpm.h" > > @@ -72,6 +73,7 @@ > > struct intel_bios_encoder_data { > > struct intel_display *display; > > > > + struct intel_ddi_buf_trans *vspeo; > > struct child_device_config child; > > struct dsc_compression_parameters_entry *dsc; > > struct list_head node; > > @@ -2628,6 +2630,30 @@ static void sanitize_device_type(struct > > intel_bios_encoder_data *devdata, > > devdata->child.device_type |= DEVICE_TYPE_NOT_HDMI_OUTPUT; } > > > > +static void allocate_vswing_preemph_override(struct > > +intel_bios_encoder_data *devdata) { > > + int num_rows = devdata->display->vbt.vspeo.num_rows; > > + union intel_ddi_buf_trans_entry *entries; > > + struct intel_ddi_buf_trans *vspeo; > > + > > + if (!intel_bios_encoder_requests_vspeo(devdata)) > > + return; > > + > > + vspeo = kzalloc_obj(*vspeo); > > + if (!vspeo) > > + return; > > + > > + entries = kzalloc_objs(*entries, num_rows); > > + if (!entries) { > > + kfree(vspeo); > > + return; > > + } > > So there is a small chance you may end up with NULL pointer dereference in > this code. > > So in the case vspeo or entries allocation fails we free the space and quietly > return. But we use > intel_bios_encoder_requests_vspeo() to decide if we want to use custom > VS/PE Tables, a function who is not aware if all the allocations happened in a > error free way. > So lets says it did not and now if anyone tries to deference devedata->vspeo > they’ll see a bug. > Maybe we can have a flag can_use in vspeo struct and fill that as true if all > allocations work out & keep it false by default. So along with > intel_bios_encoder_requests_vspeo() we also need to check this flag before > we decide if We want to go the VSPEO route or not. >
Since you create a function validate_vspeo later which does this exact thing I think I am good with this patch. LGTM, Reviewed-by: Suraj Kandpal <[email protected]> > Regards, > Suraj Kandpal > > > + > > + devdata->vspeo = vspeo; > > + devdata->vspeo->entries = entries; > > + devdata->vspeo->num_entries = num_rows; } > > + > > static void sanitize_hdmi_level_shift(struct intel_bios_encoder_data > > *devdata, > > enum port port) > > { > > @@ -2846,6 +2872,7 @@ static void parse_ddi_port(struct > > intel_bios_encoder_data *devdata) > > sanitize_dedicated_external(devdata, port); > > sanitize_device_type(devdata, port); > > sanitize_hdmi_level_shift(devdata, port); > > + allocate_vswing_preemph_override(devdata); > > } > > > > static bool has_ddi_port_info(struct intel_display *display) @@ > > -3383,6 > > +3410,11 @@ void intel_bios_driver_remove(struct intel_display > > +*display) > > list_for_each_entry_safe(devdata, nd, &display->vbt.display_devices, > > node) { > > list_del(&devdata->node); > > + > > + if (devdata->vspeo) > > + kfree(devdata->vspeo->entries); > > + > > + kfree(devdata->vspeo); > > kfree(devdata->dsc); > > kfree(devdata); > > } > > -- > > 2.45.2
